-
Notifications
You must be signed in to change notification settings - Fork 3
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 dataset folder with namespace #122
Align dataset folder with namespace #122
Conversation
Putting do-not-merge tag on until versioning PRs have been merged |
Would not merge this PR until after the release of BHoM 5.2 beta. Best merged early in 5.3. Before merging, the code should be evaluated to find any places where the library engine is called, referencing any of the datasets in here and appropriate updates made to ensure the same data is still received. |
@JosefTaylor would you have any capacity to review this so we can merge before the beta? 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay!
Dataset location works fine, though I'd consider:
"Structures\Materials\USA\Steel.json" rather than
"Structures\Materials\MaterialsUSA\Steel.json"
As for versioning, I am not sure it's working as intended- I built datasets from Main, put US Concrete and US Steel in a grasshopper script and saved it. I then built datasets from this PR, opened the script, and got this:
This may have something to do with the prototype flag, which I get when I place these datasets with the PR built:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After following the directions, versioning works great!
@BHoMBot check compliance |
thanks @JosefTaylor . As discussed offline, will aim to get this merged early next milestone, rather than in this one. Will also require the Versioning json file to get its name updated to 6.0. |
@IsakNaslundBh any update on this? |
The base branch was changed.
9087b3c
to
478253c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with script and no issues.
@BHoMBot check required |
@peterjamesnugent to confirm, the following actions are now queued:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved based on previous review and final commit (compliance change).
@IsakNaslundBh just to let you know, I have provided a |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 9 requests in the queue ahead of you. |
@BHoMBot check ready-to-merge |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 44 requests in the queue ahead of you. |
@BHoMBot check core |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 23 requests in the queue ahead of you. |
@BHoMBot check ready-to-merge |
@IsakNaslundBh to confirm, the following actions are now queued:
There are 11 requests in the queue ahead of you. |
NOTE: Depends on
Does not depend on to compile, but should not be merged before
BHoM/BHoM_Engine#2845
Issues addressed by this PR
Closes #121
Update Dataset folder to match namespace for Structures and Graphics
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/ElZxIn8tk9hEmFlsGsYwfxsBDQj3Q6jEijbald347pTANg?e=2geUir
Note 1 to test this, the C:\ProgramData\BHoM\Datasets folder needs to be cleared of any current instances of the files being migrated, as without doing so, the files on main will still exist and be picked up.
Note 2, you will need to build versioning toolkit after you have compiled this to ensure that the Upgraders are copied across to the UpgradesFile.
Changelog
Additional comments
Putting this on a separate branch name intentionally, as the versioning should be able to be merged without this PR being merged at the same time. This is raised now, before the versioning is merged, to enable simpler testing.
@peterjamesnugent @JosefTaylor putting you as reviewers as a bit of FYI that this might be happening.