Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving adapter functionality #5

Merged
merged 7 commits into from
Oct 11, 2022
Merged

Conversation

adecler
Copy link
Member

@adecler adecler commented Sep 30, 2022

NOTE: Depends on

BHoM/BHoM_Engine#2919
This is not a direct dependency (the code compiles fine without it) but this other PR solves a bug with the SetProperty component that happens in the test file.

Issues addressed by this PR

Closes #4

  • Assessing current state of the adapter and time needed to improve
  • Ability to pull from the file when it is open
  • If no range/sheet is specified in pull -> full content of first sheet
  • DataTable is very confusing to use in GH -> need to either improve its support in BHoM or replace it
  • Ability to pull a list of list instead of a DataTable
  • Ability to pull a list of CustomObjects where firs row gives the property names
  • Ability to specify the type of objects to pull

Test files

https://burohappold.sharepoint.com/:f:/s/BHoM/Eo50MIUFAS5CqkhUyfiG6PABSQBkm5R_gokTu0eGeoNxRA?e=JPFp2N

Additional comments

  • I have tried various solutions to enable to push to a file already opened in Excel but it seems that the file is properly locked by Excel for writing.
  • There is a lot more that can be done in the push but this is outside of the scope of this PR as the main focus was to get a pulling properly working. Once we have more user cases for the Push, I am happy to do another wave of development specifically around that. Right now, the main issue was to remove the need to use the DataTable.
  • Right now, the support for objects is purely limited to top level properties. Deep property explode with multi-line headers can be added in a separate PR if there is a need for non-flat structures.

@adecler adecler added the type:feature New capability or enhancement label Sep 30, 2022
@adecler adecler self-assigned this Sep 30, 2022
@adecler
Copy link
Member Author

adecler commented Sep 30, 2022

@BHoMBot check compliance
@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 30, 2022

@adecler to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check versioning

There are 15 requests in the queue ahead of you.

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally working really well, and a nice improvement, both for push and pull.

Have left 2 comments below (one typo and one slight functional tweak).

Excel_Adapter/AdapterActions/Pull.cs Outdated Show resolved Hide resolved
Excel_Engine/Create/ObjectRequest.cs Outdated Show resolved Hide resolved
@adecler adecler requested a review from IsakNaslundBh October 4, 2022 09:27
@adecler
Copy link
Member Author

adecler commented Oct 4, 2022

@BHoMBot check compliance
@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 4, 2022

@adecler to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check versioning

Copy link
Contributor

@IsakNaslundBh IsakNaslundBh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with provided test script, and works fine.

Reading the "Additional coomments" I do agree that that is out of scope for this PR, and this is a great step forward.

From code review, looks good.

Approved!

@adecler
Copy link
Member Author

adecler commented Oct 11, 2022

@BHoMBot check compliance
@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

@adecler to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check versioning

There are 4 requests in the queue ahead of you.

@adecler
Copy link
Member Author

adecler commented Oct 11, 2022

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

@adecler to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

There are 13 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

The check project-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

The check versioning has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Oct 11, 2022

The check installer has already been run previously and recorded as a successful check. This check has not been run again at this time.

@adecler adecler merged commit 19956ad into main Oct 11, 2022
@adecler adecler deleted the Excel_Toolkit-#4-ImprovingAdapter branch October 11, 2022 13:58
@FraserGreenroyd FraserGreenroyd changed the title Excel toolkit #4 improving adapter Improving adapter functionality Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving the Excel Adapter
2 participants