BHoM / BHoM_Adapter

GNU Lesser General Public License v3.0
7 stars 5 forks source link

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

Closed alelom closed 3 years ago

alelom commented 3 years ago

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

pawelbaran commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.

IsakNaslundBh commented 3 years ago

@BHoMBot check versioning

bhombot-ci[bot] commented 3 years ago
@IsakNaslundBh to confirm, `check-versioning` task is now queued.
alelom commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
alelom commented 3 years ago

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.

alelom commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
alelom commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
bhombot-ci[bot] commented 3 years ago
@IsakNaslundBh to confirm, `check-versioning` task is now queued.
bhombot-ci[bot] commented 3 years ago
@alelom just to let you know, I have provided a `check-installer` result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @JosefTaylor on ETABS_Toolkit
IsakNaslundBh commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
IsakNaslundBh commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
IsakNaslundBh commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
alelom commented 3 years ago

/azp run BHoM_Adapter.CheckInstaller

azure-pipelines[bot] commented 3 years ago
Azure Pipelines successfully started running 1 pipeline(s).
alelom commented 3 years ago

Replaced with https://github.com/BHoM/BHoM_Adapter/pull/290 due to: image