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

Align features to cover those of old File_Adapter; renaming to File_Adapter #86

Merged
merged 27 commits into from
Nov 10, 2020

Conversation

alelom
Copy link
Member

@alelom alelom commented Nov 3, 2020

NOTE: Depends on

BHoM/BHoM_Adapter#264

Issues addressed by this PR

Closes #79
Closes #80
Closes #85

Test files

All existing test files for File_Adapter and Filing_Toolkit must still work.

Please go through at least these:
https://burohappold.sharepoint.com/:f:/s/BHoM/Eub2dT2Mb85Dtvv0Y09T1I4B7pzFZHcVr5NCXRuAdSpGUQ?e=h2pDm8

⚠️ If you had compiled the Filing_Toolkit before this PR, make sure you manually delete all existing Filing_Toolkit dlls from Program Data, because a rename happened in this PR, so the old Filing_Adapter.dll etc are not deleted. This will not be an issue when deployed from the installer (because it cleans the entire ProgramData folder).

Changelog

  • Renamed to File_Adapter, File_Engine, File_oM (from "Filing").
  • Aligned features to replicate those of old File_Adapter as much as possible. Exceptions:
    • to push Datasets, this new FileAdapter requires a specific PushConfig input with DatasetSerialization set to true.
    • removed support for BSON as problematic and actually never worked/used. If requested, may be re-added later.
  • When pushing file content, the CreateOnly PushType will make the adapter append content to any existing file as explained here.
  • Switch from Regex to WildcardPattern. More reliable and simpler. Only admits * symbol, and only in the filenames. See here.

Additional comments

  • ⚠️ Before merging this, we will need a separate PR on the base BHoM_Adapter that deletes the File_Adapter.
    I will need to raise that later, as there are other PRs that need merging before that.
  • ⚠️To push Datasets, this new FileAdapter requires a specific PushConfig input with DatasetSerialization set to true. The Dataset serialisation is not proper JSON - although its extension is .json - so by default is set to false. The Library_Engine has not been updated to accept real JSON yet.

This PR replaces #81 for CI to pick correct branch name.

@alelom alelom self-assigned this Nov 3, 2020
@alelom alelom added type:compliance Non-conforming to code guidelines type:feature New capability or enhancement labels Nov 3, 2020
michaelhoehn
michaelhoehn previously approved these changes Nov 4, 2020
Copy link

@michaelhoehn michaelhoehn left a comment

Choose a reason for hiding this comment

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

Approving from a functionality perspective. I was able to run through all provided test files and have a quick test of typical uses with success.

@alelom
Copy link
Member Author

alelom commented Nov 5, 2020

@michaelhoehn I have done a simple fix for a bug - mind having a quick look again to approve? thanks!

@alelom
Copy link
Member Author

alelom commented Nov 5, 2020

@IsakNaslundBh would be great to get your review too

@alelom alelom added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Nov 5, 2020
@IsakNaslundBh
Copy link
Contributor

@michaelhoehn I have done a simple fix for a bug - mind having a quick look again to approve? thanks!
@IsakNaslundBh would be great to get your review too

The particular bug you talk about here seem to be fixed for me.

Good to mention as well, that to properly test this against older scripts, you need to manually delete all Filing_Toolkit dlls, (oM/Engine/Adapter) from Program data as the renaming means the old ones are not overwritten. Only an issue when building from source ofc, as the installer will clean it up, but though worth mentioning

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.

Push and pull assuming the old file adapter style, of giving a location to the file adapter itself, and just pushing BHoMObjects, seem to be working as expected.

The only word of caution would be if this is used to generate datasets. The Library_Engine has not been updated to accept real json, but is still assuming the line separated format. If the default values for the action config is used, when trying to generate a dataset, you will get something that the library engine can not handle basically.

Not saying that should be a blocker from merging this, just flagging that it needs to be communicated clearly to the development team.

@al-fisher
Copy link
Member

al-fisher commented Nov 6, 2020

The Library_Engine has not been updated to accept real json, but is still assuming the line separated format. If the default values for the action config is used, when trying to generate a dataset, you will get something that the library engine can not handle basically.

@IsakNaslundBh thanks for highlighting above
Agreed with your and @alelom's approach.

Have we a plan to update Library engine (and all datasets naturally) to work as valid json?
Be good to raise an issue such that this can be agreed and planned at some point in future

This will mean ultimately we can remove this "dataset" option from the File_Adapter completely - which will simply things.

@alelom
Copy link
Member Author

alelom commented Nov 6, 2020

Note that the difficulty in appending content is also that we need to check that we are appending to the same format.
I.e. you cannot push "BHoM-Dataset" text to a valid Json file, and vice-versa.
Adding a test file for that. The behaviour of the File_Adapter will be to raise error in either scenario.

@alelom alelom changed the title Align features to cover those of old File_Adapter; renaming to File_Adapter, File_Engine, File_oM Align features to cover those of old File_Adapter; renaming to File_Adapter Nov 9, 2020
@alelom
Copy link
Member Author

alelom commented Nov 9, 2020

From my tests, now everything appears fixed/working as requested from last request for changes.

Important changes

Appending mechanism

The "appending" mechanism is now more complex as it handles what explained here.
However it seems to work fine. It also handles correctly Beautified/normal JSON.

Switch from Regex to WildcardPattern

Additionally, I have switched from Regex to WildcardPattern. It is a more restrictive choice (it allows only * symbols) but it's way more reliable in this context.
Reason is, we need to process the path to check if it's a valid path; PLUS we need to detect whether the filepath contains regex symbols or not. This is tricky to do in the first place; add that this has to be done both for Sharepoint and File_Adapter paths, so it becomes basically impossible to make it work with both URLs/FileSystem paths.

Usage of wildcard

Further, to simplify the code, the wildcard is only admitted for filenames, not directory names. E.g.

  • C:\temp\*.json --> valid wildcard usage
  • C:\*\somefile.json --> invalid wildcard usage, will throw error

It is possible to admit also the second, but it would require a non negligible amount of development time.

@al-fisher
Copy link
Member

Appending is now working fine - raised a small comment above - but looking good now

al-fisher
al-fisher previously approved these changes Nov 10, 2020
@al-fisher
Copy link
Member

Approving - issues above addressed - thanks @alelom - and both File (and historic Filing) workflows working

@FraserGreenroyd FraserGreenroyd removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Nov 10, 2020
Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Happy from a CI perspective in the renaming call that the last commit only affects CI, so no functionality changes since @al-fisher last reviewed

@FraserGreenroyd FraserGreenroyd merged commit eaa59a1 into master Nov 10, 2020
@FraserGreenroyd FraserGreenroyd deleted the BHoM_Adapter-#259-SetupBeforeAction branch November 10, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:compliance Non-conforming to code guidelines type:feature New capability or enhancement
Projects
None yet
5 participants