-
Notifications
You must be signed in to change notification settings - Fork 14
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
Reflection_Engine: Final tidy up #2944
Conversation
@BHoMBot check installer |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 7 requests in the queue ahead of you. |
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 18 requests in the queue ahead of you. |
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 8 requests in the queue ahead of you. |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check unit-tests |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 16 requests in the queue ahead of you. |
@BHoMBot check required |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 9 requests in the queue ahead of you. |
The check |
@FraserGreenroyd to confirm, the following actions are now queued:
There are 3 requests in the queue ahead of you. |
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.
Changes look good.
See comments on potential further simplification and removal of some methods that I cannot really see any point in keeping :)
@FraserGreenroyd to confirm, the following actions are now queued:
There are 1 requests in the queue ahead of you. |
@FraserGreenroyd 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.
Codechanges looks good and make sense.
Made a quick test of calling the Count method from exce, including checking the versioning functioning, and no issues to report.
Approved
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
|
Issues addressed by this PR
Fixes #2734
Test files
Changelog
Additional comments
Everything that remains/wasn't moved I consider to be a part of 'reflection' - the act of the code asking questions about itself in some fashion or another.
I added documentation for any file I touched that didn't have any.
I also added some unit tests because, why not? 😄
For the
URL
unit test, I obtained all of the types from the BHoM repo, then checked all of their URLs to see which ones returned a404
- indicating the URL generated is not valid for that type. I then removed those and produced the unit test on the working ones. The reason for this is that we then have a store of the working outputs that we don't want to change - and any subsequent PRs which fix or resolve the non-working URL generations will want to check the working ones haven't broken too. Hopefully that makes sense 😄