-
Notifications
You must be signed in to change notification settings - Fork 2
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
Correcting call to AdapterId #209
Correcting call to AdapterId #209
Conversation
@BHoMBot check all |
@IsakNaslundBh sorry, I didn't understand. |
@BHoMBot check compliance |
@IsakNaslundBh to confirm, |
@BHoMBot check core |
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.
I have done a code review of this in conjunction with the previous PRs and the open PRs on the Adapter which have brought about this change. The code change presented here appears robust and in line with the requirements of the changes to the adapter.
I am approving to unblock the merging of the Adapter PRs prior to the feature freeze so that this upgraded functionality is available in the 4.1 beta being produced in a couple of weeks. I have not been able to functionality test the changes, but have discussed with @IsakNaslundBh and agree that previous tests conducted on this work related to this toolkit should be sufficient for this to merge today. Nevertheless, CI/CD are taking responsibility for the merging of this particular PR. It is highly recommended (and indeed requested in no uncertain terms) that other members of this toolkits development team (@JosefTaylor @maryannewachter @jeanlucjackson , etc.) run tests that incorporate the changes made by this PR during the next sprint (which is focused on testing in advance of the 4.1 beta release).
@IsakNaslundBh to confirm, |
@BHoMBot check installer |
@IsakNaslundBh to confirm, |
@BHoMBot check versioning |
@IsakNaslundBh to confirm, |
@BHoMBot check ready-to-merge |
@IsakNaslundBh to confirm, the task for checking if this Pull Request is ready to merge is now queued. |
NOTE: Depends on
Aligns with BHoM/BHoM_Adapter#287 again after some more code has been added. THis is also compiling to master though, so not fully dependant.
Issues addressed by this PR
Closes #208
Could not use the same branch name as for the adapter as a PR had already been opened and merged fixing the issue before. Since then, new code has been added, where the issue has been re-introduced. This should compile to master though, hence this can be and needs to be merged before the Adapter PR.
Test files
I do not have SAP2000 on my machine, but code functionality change from this PR should be non
Changelog
Additional comments