BHoM / Revit_Toolkit

A set of tools enabling exchange of information between BHoM and Revit.
GNU Lesser General Public License v3.0
27 stars 13 forks source link

Cleanup and extension of Spec and Unit queries #1498

Closed pawelbaran closed 1 month ago

pawelbaran commented 1 month ago

Issues addressed by this PR

Closes #

Test files

Changelog

Additional comments

pawelbaran commented 1 month ago

@BHoMBot check compliance

bhombot-ci[bot] commented 1 month ago
@pawelbaran to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` There are 6 requests in the queue ahead of you.
bhombot-ci[bot] commented 1 month ago
@pawelbaran just to let you know, I have provided a `check-versioning` 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 @pawelbaran on Revit_UI
pawelbaran commented 1 month ago

@BHoMBot check compliance

bhombot-ci[bot] commented 1 month ago
@pawelbaran to confirm, the following actions are now queued: - check `code-compliance` - check `documentation-compliance` - check `project-compliance` - check `branch-compliance` - check `dataset-compliance` - check `copyright-compliance` There are 5 requests in the queue ahead of you.
vietle-bh commented 1 month ago

Sorry, I just saw in another context that you wanted to "ditch naming Unit Type and Display Unit Type altogether". While DisplayUnitType should go, the latest Revit API still has UnitTypeId so I guess we can still use the term UnitType where it makes sense.

pawelbaran commented 1 month ago

Sorry, I just saw in another context that you wanted to "ditch naming Unit Type and Display Unit Type altogether". While DisplayUnitType should go, the latest Revit API still has UnitTypeId so I guess we can still use the term UnitType where it makes sense.

The problem is that the old DisplayUnitType is correspondent to UnitTypeId, while UnitType can be taken from SpecTypeId class. So, besides renaming stuff, Autodesk also managed to reuse the old concept of 'Unit Type' in the new context 🤯 This is why I got so much into 'Spec' and 'Unit' to avoid ambiguity with the old times.

vietle-bh commented 1 month ago

The problem is that the old DisplayUnitType is correspondent to UnitTypeId, while UnitType can be taken from SpecTypeId class. So, besides renaming stuff, Autodesk also managed to reuse the old concept of 'Unit Type' in the new context

I think we should only consider the new system going forward, as DisplayUnitType can be removed from our code within a year, while being descriptive on the methods can make them more useful for a long time 👍

pawelbaran commented 1 month ago

All my comments are for the same idea of making the return types more explicit. I actually find the Revit API's new unit classes very confusing so this would help me in using these methods in the future. Future devs can benefit too 👍

1707743241911

Thanks for so much thought put into the comments - indeed, the new system is so confusing. I can see that you suggest adding Type suffix to specs and units. Not sure, however, if these are needed actually- according to Revit API documentation SpecTypeId class contains constants identifying specs, i.e. the base concept is Spec, TypeId being noise added to name the class in a complex way 🙈 So what I am trying to do is demistyfying the whole thing and simplifying it to Spec and Unit.

Another thing that I tried to achieve with chosen naming was coupling to and from: SpecName query to get the name from a spec, coupled with SpecFromName to get the spec from its name. With your suggestion, we lose this coupling, which I think is worth keeping.

What do you think?

vietle-bh commented 1 month ago

Yes, let's keep your naming system. It's definitely better 👍

pawelbaran commented 1 month ago

@BHoMBot check core @BHoMBot check serialisation @BHoMBot check null-handling

bhombot-ci[bot] commented 1 month ago
@pawelbaran to confirm, the following actions are now queued: - check `core` - check `serialisation` - check `null-handling`
bhombot-ci[bot] commented 1 month ago
@pawelbaran just to let you know, I have provided a `check-versioning` 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 @pawelbaran on Revit_UI
pawelbaran commented 1 month ago

@BHoMBot check installer

bhombot-ci[bot] commented 1 month ago
@pawelbaran to confirm, the following actions are now queued: - check `installer` There are 2 requests in the queue ahead of you.
pawelbaran commented 1 month ago

@BHoMBot check ready-to-merge

bhombot-ci[bot] commented 1 month ago
@pawelbaran to confirm, the following actions are now queued: - check `ready-to-merge`