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

Adapter_Engine: Avoid signature duplication for AdapterId() methods #287

Closed
wants to merge 23 commits into from

Conversation

alelom
Copy link
Member

@alelom alelom commented Feb 11, 2021

Dependant PRs

Issues addressed by this PR

Closes #284
Closes #288

This PR revises the two methods that get the AdapterId of a specific fragment Type out of a given BHoMObject.
Both methods are useful and need to be maintained, but due to the conflict explained in #284 they had to be renamed/revised.

We have:

  1. a non generic method AdapterId(this BHoMObject, ...) that returns object. This gathers all IDs that implement a certain fragment type. If multiple IDs are found, a List is returned. If one Id is found, only that is returned.
    This method was renamed to AdapterIds<T>(this BHoMObject, ...) (note the s).
  2. a generic method that does a similar thing, but also tries to cast to the requested type <T>. This method will return an error if multiple IDs are found. This method was slightly modified to expose more useful errors/warnings and better distinguish its function from the other one. The method was not renamed.

There are only 2 Toolkits that requires alignment (SAP2000/ETABS) because they have a reference to the renamed method (1).

All other Adapter Toolkits needs to be tested to make sure that the functionality is still working as expected.

Test files

Needs testing of existing functionality on all Adapters.

Changelog

Additional comments

@alelom alelom self-assigned this Feb 11, 2021
@alelom alelom added status:WIP PR in progress and still in draft, not ready for formal review type:compliance Non-conforming to code guidelines labels Feb 11, 2021
@alelom alelom removed the status:WIP PR in progress and still in draft, not ready for formal review label Feb 12, 2021
@alelom alelom marked this pull request as ready for review February 12, 2021 11:26
@pawelbaran
Copy link
Member

I will not approve not to give a false impression that this PR has been tested extensively, but can confirm that Revit_Toolkit works perfectly fine on this branch.

@pawelbaran
Copy link
Member

Not sure whether it is fully relevant to this PR, but I have just raised #288 - being on this branch, I received a massive pushback from the users who were not able to extract any Ids without help. This tells me the UX will need further improvement once #284 is resolved.

@alelom alelom mentioned this pull request Feb 18, 2021
@alelom
Copy link
Member Author

alelom commented Feb 18, 2021

Not sure whether it is fully relevant to this PR, but I have just raised #288 - being on this branch, I received a massive pushback from the users who were not able to extract any Ids without help. This tells me the UX will need further improvement once #284 is resolved.

See my reply there.

@alelom alelom added the status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge label Feb 19, 2021
@IsakNaslundBh
Copy link
Contributor

@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 1, 2021

@IsakNaslundBh to confirm, check-versioning task is now queued.

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.

Sorry for not reviewing... Been swamped!

Code changes generally look fine to me just looking here at github, but have not tested it yet.

Would assume this would require versioning, some PreviousVersion attributes to both of the updated methods!

@alelom
Copy link
Member Author

alelom commented Mar 1, 2021

/azp run BHoM_Adapter.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alelom
Copy link
Member Author

alelom commented Mar 1, 2021

Would assume this would require versioning, some PreviousVersion attributes to both of the updated methods!

Thanks, actually only one method has been modified (AdapterId), the other (AdapterIds) is new. Will add versioning for it.

Co-authored-by: Isak Näslund <isak.naslund@burohappold.com>
Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

This works for Lusas, both Push/Pull for various objects.

I am running in to an issue when pushing to MidasCivil though on this line:

IEnumerable<IBHoMObject> objects = IRead(type, null as List<object>, actionConfig);

System.MissingMethodException: 'Method not found: '!!0 BH.Engine.Adapter.Query.AdapterId(BH.oM.Base.IBHoMObject, System.Type)'.'

Basically it's the first read before it pushes (so should just be empty).

System.MissingMethodException
HResult=0x80131513
Message=Method not found: '!!0 BH.Engine.Adapter.Query.AdapterId(BH.oM.Base.IBHoMObject, System.Type)'.
Source=MidasCivil_Adapter
StackTrace:
at BH.Adapter.MidasCivil.MidasCivilAdapter.ReadBars(List`1 ids)
at BH.Adapter.MidasCivil.MidasCivilAdapter.IRead(Type type, IList ids, ActionConfig actionConfig)
at BH.Adapter.BHoMAdapter.Read(Type type, String tag, ActionConfig actionConfig) in C:\Users\pnugent\Documents\GitHub\BHoM_Adapter\BHoM_Adapter\CRUD\IRead.cs:line 100

Script is here if you want to give it a try and debug:
https://burohappold.sharepoint.com/:f:/r/sites/BHoM/02_Current/12_Scripts/01_Issue/Archive/MidasCivil_Toolkit/MidasCivil_Toolkit-%23263/Elements?csf=1&web=1&e=erZTfB

Copy link
Member

@peterjamesnugent peterjamesnugent left a comment

Choose a reason for hiding this comment

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

Tested for both Lusas and Midas, able to Push/Pull a number of different objects.

IsakNaslundBh
IsakNaslundBh previously approved these changes Mar 12, 2021
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.

Works fine now!

Need to get BHoM/SAP2000_Toolkit#209 merged first, then this should be good to go!

@IsakNaslundBh
Copy link
Contributor

/azp run BHoM_Adapter.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@alelom
Copy link
Member Author

alelom commented Mar 12, 2021

/azp run BHoM_Adapter.CheckInstaller

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Re-approving

alelom added a commit that referenced this pull request Mar 15, 2021
Due to merge conflicts. See version history on that PR.
#287
@alelom
Copy link
Member Author

alelom commented Mar 15, 2021

Replaced with #290 due to:
image

@alelom alelom closed this Mar 15, 2021
IsakNaslundBh pushed a commit that referenced this pull request Mar 23, 2021
Due to merge conflicts. See version history on that PR.
#287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:do-not-merge For instance, test PR, for discussion, or dependant PRs not ready for merge type:compliance Non-conforming to code guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the Id query Two methods AdapterId with the exact same name and list of parameters
4 participants