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-refactor the ReplaceInMemory #106

Closed
Tracked by #84
alelom opened this issue Jul 2, 2019 · 1 comment
Closed
Tracked by #84

BHoM_Adapter: refactoring-refactor the ReplaceInMemory #106

alelom opened this issue Jul 2, 2019 · 1 comment
Assignees
Labels
severity:medium Slows progress, but workaround is possible type:compliance Non-conforming to code guidelines

Comments

@alelom
Copy link
Member

alelom commented Jul 2, 2019

The ReplaceInMemory has an counterintuitive structure that needs to be refactored.

Issue 1 - Fix missed VennDiagram bug

Fix #88.
image

Issue 2 - structure of ReplaceInMemory and call to Create()

Counter-intuitively, the Create method will have to take care not only to create (export) these objects, but it is assumed also to be deleting any object existing in the model.
To distinguish this case, there is a boolean argument replaceAll to be set to true when calling Create(), which is not good.

Step by step explanation:

  1. The Method assumes that the Create method will delete all the existing objects from the model.
  2. In order to do 1., an additional Boolean replaceAll was added to the Create method. When activated, it is assumed that the Create will be deleting objects. This parameter is used exclusively for this purpose.
  3. Since 2. is always required when doing ReplaceInMemory, its call in the main Replace method hard-codes replaceAll to true (here renamed to overwriteObjects):
    if (Config.ProcessInMemory)
    {
    objectsToCreate = ReplaceInMemory(newObjects, existing, tag);
    overwriteObjects = true;
    }

    So Create will get called with ReplaceAll set to true when ReplaceInMemory was used:
    if (!Create(objectsToCreate, overwriteObjects))

    Which is definitely not nice.

I have raised an issue to collection all related toolkit issues: #114

Proposed solution

The replaceAll parameter should be removed by the Create method signature.
Any deletion should be preformed using the Delete method.

Issue 3 - usage of ReplaceInMemory

ReplaceInMemory involves the use of two AdapterConfig properties:

  • ProcessInMemory needs to be true in order for ReplaceInMemory to be called.
  • MergeWithComparer to have the ReplaceInMemory using the comparers to do the merge. If this is set to false, the ReplaceInMemory becomes a sort of "CreateOnly" method (except it does not Create the objects with the same tag currently Pushed, a rather cryptic behaviour).

Now, consider that in order to use ReplaceInMemory as intended, you need to:

  • set ProcessInMemory to true, AND
  • implement the Create() method as per point 1.

Let's find out about the usage of the ReplaceInMemory then:

image

➡️ The FileAdapter is the only one that uses ReplaceInMemory as intended.

Another 4 adapters are specifying ProcessInMemory as true, but since they are not implementing Create(), they are not using this ReplaceInMemory as intended. These are:
Unreal_Toolkit; Filing_Toolkit; Civil_Toolkit; GitHub_Toolkit.

➡️ For those, the theory is that people have been using ReplaceInMemory as a sort of CreateOnly method, disregarding tags.

➡️ AdapterConfig.MergeWithComparer is never used.

Proposed solution

  • Delete AdapterConfig.MergeWithComparer. ReplaceInMemory should always use comparers, like ReplaceThroughAPI.
  • Modify Unreal_Toolkit, Filing_Toolkit, Civil_Toolkit, GitHub_Toolkit to use a CreateOnly method instead of ReplaceInMemory.
  • Modify the File_Adapter. For him, the Create() parameter replaceAll is always set to false. Some of the code needs to be cleaned.
@alelom alelom added severity:medium Slows progress, but workaround is possible type:compliance Non-conforming to code guidelines labels Jul 2, 2019
@alelom alelom self-assigned this Jul 2, 2019
@alelom alelom changed the title BHoM_Adapter: refactor the ReplaceInMemory BHoM_Adapter: refactoring-refactor the ReplaceInMemory Jul 3, 2019
@alelom alelom added this to the BHoM 2.5 β MVP milestone Aug 20, 2019
@alelom
Copy link
Member Author

alelom commented Dec 2, 2019

Closed by #151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:medium Slows progress, but workaround is possible type:compliance Non-conforming to code guidelines
Projects
None yet
Development

No branches or pull requests

2 participants