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

Added "Setup" methods to do the Actions input setup that was in BHoM_UI #264

Merged
merged 14 commits into from
Nov 6, 2020

Conversation

alelom
Copy link
Member

@alelom alelom commented Oct 30, 2020

NOTE: Depends on

BHoM/BHoM_UI#329

Issues addressed by this PR

Closes #259

Moves the logic related to the Adapter Actions setup from the BHoM_UI in the Adapter.
Advantages:

I need this quite urgently now because of some needed changes in other toolkits (File_Adapter).

Test files

Changelog

Additional comments

@alelom alelom self-assigned this Oct 30, 2020
@alelom alelom added type:compliance Non-conforming to code guidelines type:feature New capability or enhancement labels Oct 30, 2020
@alelom alelom added the severity:critical No workaround exists. Essential to continue label Oct 30, 2020
Copy link
Member

@adecler adecler left a comment

Choose a reason for hiding this comment

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

Most of the code in here is aimed at handling null cases. So wondering if it wouldn't make more sense to have that directly into to core method instead of a separate one. Genuine question though as I haven't been involved with the adapters long enough that my question might have an obvious answer.

@alelom
Copy link
Member Author

alelom commented Nov 3, 2020

Most of the code in here is aimed at handling null cases. So wondering if it wouldn't make more sense to have that directly into to core method instead of a separate one. Genuine question though as I haven't been involved with the adapters long enough that my question might have an obvious answer.

The main point of having the logic here is to allow overriding by Toolkits. This gives the flexibility of adding checking/verification/additional settings to the "SetUp" methods, or replacing entirely what we do at the base. This is useful in several scenarios.

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.

Like the action config and request stuff, but think the adapter checking for Move as well as the object checking for push is one step to many. Those to me are clear parts of the push, not something that is simply cleaned up a bit before.

BHoM_Adapter/AdapterActions/Pull.cs Outdated Show resolved Hide resolved
BHoM_Adapter/AdapterActions/Pull.cs Show resolved Hide resolved
BHoM_Adapter/AdapterActions/Move.cs Outdated Show resolved Hide resolved
BHoM_Adapter/AdapterActions/Push.cs Outdated Show resolved Hide resolved
@IsakNaslundBh
Copy link
Contributor

Most of the code in here is aimed at handling null cases. So wondering if it wouldn't make more sense to have that directly into to core method instead of a separate one. Genuine question though as I haven't been involved with the adapters long enough that my question might have an obvious answer.

To expand a bit on alessios answer, it is more about defaults and converts than null checks, and making it possible for each adapter to handle that differently. For example, being able to convert a string, formatted in a specific way into a request that is friendly to that particular adapter, but non-sensical outside that scope.

As for the ActionConfigs, could be used by RevitToolkit for example, that make use of that mechanism a lot, to directly instantiate the appropriate ActionConfig required by a particular action, such as Pull.

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.

some small additional tweaks

BHoM_Adapter/AdapterActions/Pull.cs Outdated Show resolved Hide resolved
BHoM_Adapter/AdapterActions/Pull.cs Outdated Show resolved Hide resolved
File_Adapter/AdapterActions/Push.cs Outdated Show resolved Hide resolved
alelom and others added 5 commits November 4, 2020 09:03
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
@alelom alelom added status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge and removed status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge labels Nov 5, 2020
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.

now LGTM. Think this is a good balance!

@alelom alelom changed the title Added SetupThen[ActionName] methods to do the setup logic that used to be in the BHoM_UI Added "Setup" methods to do the Actions input setup logic that used to be in the BHoM_UI Nov 6, 2020
@alelom alelom changed the title Added "Setup" methods to do the Actions input setup logic that used to be in the BHoM_UI Added "Setup" methods to do the Actions input setup that used to be in the BHoM_UI Nov 6, 2020
@alelom alelom changed the title Added "Setup" methods to do the Actions input setup that used to be in the BHoM_UI Added "Setup" methods to do the Actions input setup that used to be in BHoM_UI Nov 6, 2020
@alelom alelom changed the title Added "Setup" methods to do the Actions input setup that used to be in BHoM_UI Added "Setup" methods to do the Actions input setup that was in BHoM_UI Nov 6, 2020
@alelom
Copy link
Member Author

alelom commented Nov 6, 2020

/azp run BHoM_Adapter.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:critical No workaround exists. Essential to continue type:compliance Non-conforming to code guidelines type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move all logic inside Adapter Action UI components to the Adapter
3 participants