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

BHoM_Adapter refactoring Lvl3_01 - workaround to have the CRUD working for non-BHoMObjects #142

Merged
merged 11 commits into from
Oct 8, 2019

Conversation

alelom
Copy link
Member

@alelom alelom commented Oct 4, 2019

Issues addressed by this PR

Closes #140
Also adds some comments and a few renamings for clarity.

Test script

https://burohappold.sharepoint.com/:u:/s/BHoM/ERekpoPskXFOuC66VS7yG74BFNT5HaGuqKBFTcOj_qRvfw?e=qCF92Q

Changelog

  • Added a WrapNonBHoMObjects() method. Wraps non-BHoMObjects into Custom BHoM Objects so they can make use of the Adapter Replace/Crud methods as the BHoMObjects (they get an ID, CustomData, etc).
  • Added AdapterConfig.WrapNonBHoMObjects that defines default Toolkit behaviour for that method. Defaults to false, as it should be used only in few specific Toolkits.
  • Added a push config boolean option that overrides the default AdapterConfig setting if specified, so the user has control over the specific Push case.

@alelom alelom self-assigned this Oct 4, 2019
@alelom alelom added the type:feature New capability or enhancement label Oct 4, 2019
@alelom alelom added this to the BHoM 3.0 β MVP milestone Oct 4, 2019
Comment on lines 66 to 70
// Wrap non-BHoM objects into a Custom BHoMObject to make them work as BHoMObjects.
List<IObject> objectsToPush = Modify.WrapNonBHoMObjects(objects, Config, tag, pushConfig).ToList();

// Clone the objects for immutability in the UI. CloneBeforePush should always be true, except for very specific cases.
objectsToPush = Config.CloneBeforePush ? objects.Select(x => x.DeepClone()).ToList() : objects.ToList();
Copy link
Member Author

Choose a reason for hiding this comment

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

@IsakNaslundBh the reason for WrapNonBHoMObjects() checking the boolean WrapNonBHoMObjects inside the method itself is because this way we can freely override the default Adapter setting through the Push config.

@alelom alelom added severity:medium Slows progress, but workaround is possible severity:critical No workaround exists. Essential to continue and removed severity:medium Slows progress, but workaround is possible labels Oct 7, 2019
@alelom alelom changed the title BHoM_Adapter refactoring Lvl3 - part01 BHoM_Adapter refactoring Lvl3_01 - workaround to have the CRUD working for non-BHoMObjects Oct 7, 2019
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.

Small error in picking correct objects when wrapping is done

File_Adapter/CRUD/Push.cs Outdated Show resolved Hide resolved
BHoM_Adapter/BHoMAdapter.cs Outdated Show resolved Hide resolved
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.

LGTM.

Works as expected

@alelom alelom merged commit cfdf43d into master Oct 8, 2019
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:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BHoM_Adapter: capability for geometries (generally, IObjects) to have IBHoMObject properties attached
2 participants