Adapter refactoring Level 04 #164

alelom · 2019-11-14T16:42:14Z

Issues addressed by this PR

Changes on Config and Settings: Closes #45 Closes #108

Changes on Delete action: Closes #138 Closes #156

Retrocompatibility: Closes #158

MapSpecialProperties: Closes #168

Project and folders rearrangements/tidy up: Closes #170, Closes #169, Closes #171

Test files

Test files for specific Toolkits to be found in their related alignment PR.

At the moment, you can only find GSA: BHoM/GSA_Toolkit#130

Changelog

CRUD base actions are called ICreate, IDelete, etc in order to follow Engine convention of interface methods "to be implemented"
New field m_AdapterSettings. This is where default settings to be used for the Adapter can be set once per build.
Replaced the config Dictionary<string,object> input for all Adapter Actions with a new class ActionConfig. It contains configs common to all Toolkits. It can be inherited in specific Toolkits to create other adapter-specific Configs.
Delete has been renamed to Remove.
UpdateProperty has been renamed to UpdateTag. It only handles tag which is the only correct use. Other cases must fall in Push scenarios.
MapSpecialProperties has been splitted in 2 separate methods: CopyBHoMObjectProperties that copies only IBHoMObject basic properties over; ICopyTypeSpecificProperties that's meant to copy type specific properties. Specific implementation of that must be done in specific Toolkits.
Introduction of PushType (and PullType) enum. Allow to select the type of Push being done (FullCRUD, DeleteThenCreate, CreateOnly, etc).
Added a Push Type and code implementation: DeleteThenCreate
Folder and files reorganisation:
- Created new Adapter_oM project
- Created new Adapter_Engine project
- All methods and class definition that could be moved up to those project have been moved. Exceptions are only those methods that have to be left in the BHoMAdapter partial class in order to allow overrides in specific Toolkits.
- General widespread tidy up of the code.
Introduced the concept of Adapter Modules. See this comment for a description.

…Only`, `DependenciesCreateOnly`, `DependenciesUpdateOnly`

…rence (does not return a new List)

There is not a single use case where they are used with non-IBHoMObjects anyway. This clarifies that they are meant to be implemented as part as the full CRUD

…ption

…ss name. #163 No need to store its value as a hard-coded value somewhere, like "Robot_id". The initialisation had to be done inside a new BHoMAdapter constructor, because the identifier `this` is not available in the auto-property initialisers.

… 2) Added ActionConfig global property and logic to reset it at the start of each AdapterAction (except Move).

…now in BHoM_UI (except for the Move, which must have it here)

…w projects.

- UpdateTags is not a CRUD method because there is no CRUD action involved; it's only used to update (if existing) a collection of tags to be stored in a private field of the specific adapter. Moved in new folder "SupportMethods". - MapSpecialProperties might end up in "SupportMethods" too - Refactored the Push to allow for AdapterSettings to be overridden with more ActionConfig, especially for the PushType - Added a PushType for `DeleteAllThenCreate` that should clean the exported model and then create stuff in it. - Added method in FullCRUD that does the previous. Uses the DiffingEngine "HashComparing", but only to avoid Deleting and then Creating the same stuff. - MapSpecialProperties must be abstracted more. Renamed as `PortProperties`. Added generic method that does the same thing, but for any object and given a propertyName. - When the new PortProperties is called by the adapter, specific adapter implementation can say what properties should be ported. This work has to be finished.

The generic `PortProperties` might be useless. Being a feature addition I'll keep it.

…Only

epignatelli · 2020-01-23T11:00:46Z

BHoM_Adapter/AdapterActions/Move.cs

-        // Performs a Pull and then a Push. Useful to move data between two different software without passing it through the UI.
-        public virtual bool Move(BHoMAdapter to, IRequest request, Dictionary<string, object> pullConfig = null, Dictionary<string, object> pushConfig = null)
+        [Description("Performs a Pull and then a Push. Useful to move data between two different software without passing it through the UI.")]
+        public virtual bool Move(IBHoMAdapter to, IRequest request, 


I feel that this is actually a copy rather than a move at this point.

epignatelli

That's a big refactoring! I felt it made the HTTPAdapter simpler when amending the code do comply with this.

Some observations I made with @alelom while reviewing this.
The come with a green tick because I believe the best time to do these is in the next wave.

When performing an action, like a Push or a Pull, I feel that the information to include are sparser than they can be. I.e. PullType can be a property of the ActionConfig. This takes us to specifty the ActionConfig into PullConfig, PushConfig, ExecuteConfig, MoveConfig, and RemoveConfig. They all inherit from ActionConfig. All of this allows to keep the information more compressed and I feel that's more intuitive.
In the light of 1., the ActionConfig can be removed from the Create, as it is never used.
I never know if to override the Pull or the Read or any other one, when I need to perform a task. I prefer to be channeled to override only one or the other, otherwise the implementer is completely lost in there. My suggestion is to use the pattern Grasshopper uses for the component. To disable the override of a Pull or a Push, and add two functions BeforeAction and AfterAction, the same way Grasshopper implements BeforeSolveInstance and AfterSolveIntance. This way if you need to wrap your objects around BHoMObjects or anything else, you can override these two.

FraserGreenroyd

Based on the tests I have run on my toolkits as part of the refactoring process, this is good to me. I have not reviewed the code but can see from comments that others have.

alelom · 2020-01-24T09:42:12Z

My suggestion is to use the pattern Grasshopper uses for the component. To disable the override of a Pull or a Push, and add two functions BeforeAction and AfterAction, the same way Grasshopper implements BeforeSolveInstance and AfterSolveIntance. This way if you need to wrap your objects around BHoMObjects or anything else, you can override these two.

This is really a good suggestion! It would simplify things. However, we must also consider that overriding the "core" action might be needed in some cases, as brought up by @adecler.
But if we include a "BeforeSolve" and "AfterSolve", we could at least make this "unadvisable", limiting overrides to only the strictly neededed cases. Like saying, if you override the action, then you need to make sure you understand what is needed for it to work properly: like implementing some checks that should come with it.

FraserGreenroyd · 2020-01-24T09:46:54Z

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines · 2020-01-24T09:47:06Z

Azure Pipelines successfully started running 1 pipeline(s).

al-fisher

Happy to approve this. Tested on a number of different Adapter_Toolkits.
And in addition approval based on the very valuable restructuring of the Adapter itself, and the successful passing reviews on the many dependant PRs.

alelom added 12 commits November 11, 2019 12:42

Moved ReadResult into its own file

df7b0fe

New folder structure: CRUDCallers and Extra. New methods: `Create…

6a97254

…Only`, `DependenciesCreateOnly`, `DependenciesUpdateOnly`

WrapNonBHoMObjects now accepts object and modifies the list by refe…

aaddddc

…rence (does not return a new List)

Create and Update now accept only IBHoMObjects for consistency

160436d

There is not a single use case where they are used with non-IBHoMObjects anyway. This clarifies that they are meant to be implemented as part as the full CRUD

Rename UpdateObjects to Update

bb753ae

Minor comment adjustments

7184927

Added PushOption enum parameter to Push. Push uses it.

0b1bd6c

1. Renamed Config to AdapterSettings. 2. Added PushOption & PullO…

43efc5a

…ption

Tidy up and corrections

c5ca3ee

Renamed the AdapterAction Delete to Remove.

03dc8e8

1) Changes in folder/files arrangement following chat with @al-fisher.…

0c24574

… 2) Added ActionConfig global property and logic to reset it at the start of each AdapterAction (except Move).

alelom self-assigned this Nov 14, 2019

alelom added the status:WIP PR in progress and still in draft, not ready for formal review label Nov 14, 2019

Removed init logic for the config Dictionary from the Actions. It is …

92a2b01

…now in BHoM_UI (except for the Move, which must have it here)

This was referenced Nov 14, 2019

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/BHoM_UI#167

Merged

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/Grasshopper_UI#440

Merged

alelom added 5 commits November 15, 2019 10:55

Added Adapter_oM and Adapter_Engine projects -> moved files. Comments.

ce1f69f

Corrected default namespaces, build folder and copy locals for the ne…

d121a5c

…w projects.

Merge branch 'master' into BHoM_Adapter-refactoringLvl04-01

0ffe1fd

Minor rewriting of a comment

8784c8b

Minor constructor and properties tidy up.

14a2663

adecler mentioned this pull request Nov 20, 2019

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/Socket_Toolkit#57

Merged

alelom added 7 commits November 20, 2019 11:40

Minor comment tidy-ups

73c2434

First introduction of Adapter ID Fragment

5ffa21c

New take on IdFragment.

feb314b

Simpler iteration on AdapterIdFragment.

5e2176d

Tidy up.

fdb1c09

The generic `PortProperties` might be useless. Being a feature addition I'll keep it.

Comment in PortProperties

9990928

alelom and others added 7 commits January 22, 2020 11:15

Added descriptions.

7ce164f

Update create only to make it able to be recursive

a84f6e2

Adding missing action configs in UpdateOnly and FullCRUD

8bfeb08

Added few missing ActionConfig parameters.

9f2cfc1

Added few more descriptions and renamed the bool "distinct" in Create…

c63c7df

…Only

corrected tagging

ebe1a66

Added missing ActionConfig parameters in ReadResult methods

5de00c7

JosefTaylor previously approved these changes Jan 22, 2020

View reviewed changes

peterjamesnugent mentioned this pull request Jan 22, 2020

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/Lusas_Toolkit#210

Merged

Update Pull.cs

c78c00a

alelom dismissed JosefTaylor’s stale review via c78c00a January 22, 2020 16:54

Update Pull.cs

515977e

peterjamesnugent mentioned this pull request Jan 22, 2020

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/MidasCivil_Toolkit#166

Merged

epignatelli mentioned this pull request Jan 23, 2020

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/HTTP_Toolkit#28

Merged

epignatelli removed their request for review January 23, 2020 10:55

epignatelli reviewed Jan 23, 2020

View reviewed changes

epignatelli self-requested a review January 23, 2020 11:30

epignatelli approved these changes Jan 23, 2020

View reviewed changes

FraserGreenroyd mentioned this pull request Jan 23, 2020

Aligning with changes in BHoMAdapter Refactoring Level 04 BHoM/RFEM5_Toolkit#40

Merged

FraserGreenroyd approved these changes Jan 24, 2020

View reviewed changes

al-fisher changed the title ~~Adapter refactoring Level04~~ Adapter refactoring Level 04 Jan 24, 2020

al-fisher added the type:compliance Non-conforming to code guidelines label Jan 24, 2020

IsakNaslundBh approved these changes Jan 24, 2020

View reviewed changes

al-fisher approved these changes Jan 24, 2020

View reviewed changes

FraserGreenroyd removed the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Jan 24, 2020

alelom merged commit 610e130 into master Jan 24, 2020

alelom deleted the BHoM_Adapter-refactoringLvl04-01 branch January 24, 2020 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapter refactoring Level 04 #164

Adapter refactoring Level 04 #164

alelom commented Nov 14, 2019 •

edited

Loading

epignatelli Jan 23, 2020

epignatelli left a comment

FraserGreenroyd left a comment

alelom commented Jan 24, 2020

FraserGreenroyd commented Jan 24, 2020

azure-pipelines bot commented Jan 24, 2020

al-fisher left a comment

Adapter refactoring Level 04 #164

Adapter refactoring Level 04 #164

Conversation

alelom commented Nov 14, 2019 • edited Loading

Alignment instructions

NOTE: DEPENDS ON

NOTE: BRANCHES DEPENDANT ON THIS (needed for testing)

Issues addressed by this PR

Test files

Changelog

epignatelli Jan 23, 2020

Choose a reason for hiding this comment

epignatelli left a comment

Choose a reason for hiding this comment

FraserGreenroyd left a comment

Choose a reason for hiding this comment

alelom commented Jan 24, 2020

FraserGreenroyd commented Jan 24, 2020

azure-pipelines bot commented Jan 24, 2020

al-fisher left a comment

Choose a reason for hiding this comment

alelom commented Nov 14, 2019 •

edited

Loading