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

Aligning with changes in BHoMAdapter Refactoring Level 04 #293

Merged
merged 32 commits into from
Jan 24, 2020

Conversation

IsakNaslundBh
Copy link
Contributor

@IsakNaslundBh IsakNaslundBh added status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:external-api-changes Imposed changes, including from dependency across other BHoM repos labels Jan 13, 2020
@IsakNaslundBh IsakNaslundBh added this to the BHoM 3.1 β MVP milestone Jan 13, 2020
@IsakNaslundBh IsakNaslundBh self-assigned this Jan 13, 2020
@alelom
Copy link
Member

alelom commented Jan 23, 2020

Code LGTM. Tested Push. Need to finish testing tomorrow morning.

Copy link
Contributor

@kThorsager kThorsager left a comment

Choose a reason for hiding this comment

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

Looked through the code and did some minor tests,

RobotAdapter.cs has some private methods under its public header, minor thing but still

There is an enum: command in Robot_oM, but it's empty? If whatever planed functionality it had has been take over by the adapter commands it should be removed.

In Execute Analyse ignore warnings is set to true, but warnings still appear (noticed for instability, lack of section and when results are overwritten)

AnalyseLoadCases does sometimes run more commands than it's told, as in I have three loadcases, run one through AnalyseLoadCases and get the results for the one I want and an other.
If I run the one that's always added, that is the only one.

And If I try to re-run another load case, then I only get the results for the one that always appear or both, depending on load case.
Not sure whats going on

@IsakNaslundBh
Copy link
Contributor Author

Thanks @kThorsager

RobotAdapter.cs has some private methods under its public header, minor thing but still

think those are fine. Very robot specific methods right now used by various parts of the adapter. To me those makes sense in there at least.

There is an enum: command in Robot_oM, but it's empty? If whatever planed functionality it had has been take over by the adapter commands it should be removed.

Good catch! Might leave this to a general refactoring/cleanup of the toolkit as not really affected by this though

In Execute Analyse ignore warnings is set to true, but warnings still appear (noticed for instability, lack of section and when results are overwritten)

AnalyseLoadCases does sometimes run more commands than it's told, as in I have three loadcases, run one through AnalyseLoadCases and get the results for the one I want and an other.
If I run the one that's always added, that is the only one.

And If I try to re-run another load case, then I only get the results for the one that always appear or both, depending on load case.
Not sure whats going on

Will try to have a look at this if resolveable quickly before merge deadline

@alelom
Copy link
Member

alelom commented Jan 24, 2020

Tested so far:

all working fine.

image

@alelom alelom requested a review from kThorsager January 24, 2020 11:01
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Approved in light of latest changes by @IsakNaslundBh . If other issues will be found, we'll raise a bug issue.

Copy link
Contributor

@kThorsager kThorsager left a comment

Choose a reason for hiding this comment

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

While all of my issues haven't been resolved, I have been promised that issues will be raised and since they were minor and not necessarily related to this refactoring I would say that this looks good, (and works).

@FraserGreenroyd FraserGreenroyd removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Jan 24, 2020
@IsakNaslundBh IsakNaslundBh merged commit 6bfbf7a into master Jan 24, 2020
@IsakNaslundBh IsakNaslundBh deleted the BHoM_Adapter-refactoringLvl04-01 branch January 24, 2020 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:external-api-changes Imposed changes, including from dependency across other BHoM repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Robot_Toolkit: Align to changes for BHoM_Adapter refactoring level 04
4 participants