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.

Sign up for GitHub

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

Jump to bottom

BHoM_Adapter - refactoring - Level 3 changes #151

Merged
merged 32 commits into from
Nov 1, 2019
Merged

BHoM_Adapter - refactoring - Level 3 changes #151

merged 32 commits into from
Nov 1, 2019

Conversation

alelom
Copy link
Member

@alelom alelom commented Oct 18, 2019 •
edited
Loading

Issues addressed by this PR

Closes #114 ,Closes #87, closes #147, closes #155, closes #113, closes #109, closes #148, closes #149

NOTE: This PR will break ALL TOOLKITS that include an Adapter project

However, no script will break due to this.

There is an admin issue that explains the steps needed to align with this change.

Changelog

  • Adapter Actions:

    • All Adapter actions (Push, Pull, etc) now live in different files (Push.cs, Pull.cs, etc) in a folder named AdapterActions.
      • The Delete Adapter Action now lives in a file (temporarily?) named Remove.cs, while awaiting on the decision on a better naming to distinguish it from the CRUD delete. Discussion here.
    • Removed the UpdateProperty Adapter Action, as discussed here.
    • Added a dispatch pattern in the Pull() that calls the appropriate Read()/ReadResults() method depending on the base parent type of the provided request, using as dynamic.

  • CRUD methods:

    • Removed the parameter replaceAll from the Create() method signature. It was never properly used.
    • Changes in the Read/ReadResults methods:
      • Added empty protected virtual Read(IRequest) Wrapper virtual method
      • Added empty protected virtual ReadResult(IRequest) Wrapper virtual method
      • These Read methods will now be required in single Toolkit implementations in order for the Read to work with different types of Requests. The appropriate read method will now be called by the as dynamic pattern in the Pull, as introduced above in the "Adapter Actions" changes.
    • Made all CRUD Wrapper Methods virtual for consistency.
    • Tidy up in the arrangement of the CRUD methods:
      • Moved the "Protected Abstract CRUD Methods" in their respective files in the CRUD folder (Create.cs, Read.cs, etc.). From now on, we will call those Basic CRUD methods.
      • Renamed the UpdateOnly.cs file to Update.cs.
      • Moved the UpdateProperty in the same file as the UpdateOnly, Update.cs.
      • Added comment sections to clearly separate Basic CRUD methods and Wrapper CRUD methods. Wrapper methods are now those that are meant to add some functionality and then call the Basic CRUD method.
      • Added comments to clarify when such or such method should be implemented/overridden

  • Adapter properties:

    • Deleted the superseded ErrorLog Adapter property.
    • Changes to the Adapter Config public property:
      • Renamed SeparateProperties to HandleDependencies. Changed its default value from False to True.
      • Changed the default value of ProcessInMemory from True to False.
      • Removed MergeWithComparer property. It was never used.

  • Adapter other methods:

    • Renamed Replace() method to CRUD()

  • File_Adapter:

    • File_Adapter: changes to comply with the changes above.
    • File_Adapter: removed the Legacy folder and its content (was all commented/unused code).

Additional notes

Needs all these PRs to be merged in master:

@alelom
Removed replaceAll param from Create signature.
7137328
@alelom
Changes required in the FileAdapter.
115e4d4
@alelom
Rename Replace method to CRUD
3e1fa6f
@alelom
Rename Config.SeparateProperties to HandleDependencies
f4037c1
@alelom
Moved Protected Abstract CRUD methods into their own respective files…
4af6bcc 
… in the CRUD folder
@alelom
Added Read method to work with generic IRequest; updated Pull to call it
482c8e3
@alelom
Started reviewing how the Pull, Read and Request work
71d550a
@alelom
Method tidy up in CRUD folder; added ReadResultCollection and ReadRes…
996db98 
…ults methods accepting generic IRequest instead of FilterRequest
@alelom
Rearrangement of methods. Consistency for methods with IRequests. Rem…
967cadc 
…oved UpdateProperty Action. Lots of comments.

Only consistency for IRequest missing is in the UpdateProperty, that at this point is too messy to do.
@alelom alelom self-assigned this Oct 18, 2019
@alelom alelom added the status:WIP PR in progress and still in draft, not ready for formal review label Oct 18, 2019
@alelom alelom added this to the BHoM 3.0 β MVP milestone Oct 18, 2019
@alelom alelom mentioned this pull request Oct 24, 2019
Closed
This was referenced Oct 24, 2019
Merged
Merged
@alelom
Made all CRUD Wrapper Methods virtual for consistency. Only 2 weren…
58ea383 
…'t already.
@alelom
Changes to the Adapter Config public property.
91b8c60 
1. Removed `MergeWithComparer` which was never used. It was only referenced in `ReplaceInMemory()` but that case was never true due to Toolkit implementations. Added a bool `mergeWithCompararer` input parameter in the `ReplaceInMemory()` method instead.
2. Changed `ProcessInMemory` default value to `false`.
3. Changed `HandleDependencies` (formerly `SeparateProperties`) default value to `true`.
@JosefTaylor JosefTaylor mentioned this pull request Oct 28, 2019
Merged
@alelom alelom removed the status:WIP PR in progress and still in draft, not ready for formal review label Oct 29, 2019
@alelom alelom marked this pull request as ready for review October 29, 2019 14:05
@alelom alelom mentioned this pull request Oct 31, 2019
Closed
@FraserGreenroyd FraserGreenroyd mentioned this pull request Oct 31, 2019
Closed
This was referenced Oct 31, 2019
Closed
Merged
Closed
This was referenced Oct 31, 2019
Merged
Merged
Merged
Merged
@alelom alelom mentioned this pull request Oct 31, 2019
Merged
This was referenced Oct 31, 2019
Merged
Merged
This was referenced Oct 31, 2019
Merged
Closed
FraserGreenroyd
FraserGreenroyd approved these changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

LGTM

This was referenced Nov 1, 2019
Closed
Merged
@peterjamesnugent peterjamesnugent mentioned this pull request Nov 1, 2019
Merged
@alelom alelom merged commit 7b6e246 into master Nov 1, 2019
@peterjamesnugent peterjamesnugent deleted the BHoM_Adapter-refactoringLvl3-01 branch November 1, 2019 15:47
This was referenced Nov 4, 2019
Closed
Closed
This was referenced Dec 2, 2019
Closed
Closed
This was referenced Dec 19, 2019
Closed
Closed
@alelom alelom mentioned this pull request Mar 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IsakNaslundBh IsakNaslundBh IsakNaslundBh left review comments

@FraserGreenroyd FraserGreenroyd FraserGreenroyd approved these changes

@al-fisher al-fisher Awaiting requested review from al-fisher

@adecler adecler Awaiting requested review from adecler

Assignees

@alelom alelom

Labels
None yet
Projects
None yet
Milestone
BHoM 3.0 β MVP
3 participants
@alelom @FraserGreenroyd @IsakNaslundBh