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

Cleanup and extension of Spec and Unit queries #1498

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

pawelbaran
Copy link
Member

Issues addressed by this PR

Closes #

Test files

Changelog

  • renamed UnitTypeFromPropertyName to SpecFromName and UnitTypePropertyName to SpecName
  • added UnitName and UnitFromName

Additional comments

@pawelbaran pawelbaran added the type:feature New capability or enhancement label Aug 10, 2024
@pawelbaran pawelbaran self-assigned this Aug 10, 2024
@pawelbaran
Copy link
Member Author

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Aug 10, 2024

@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.

Copy link

bhombot-ci bot commented Aug 12, 2024

@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
Copy link
Member Author

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Aug 12, 2024

@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.

Copy link
Contributor

@vietle-bh vietle-bh left a comment

Choose a reason for hiding this comment

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

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

Revit_Core_Engine/Query/UnitFromName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/UnitFromName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/UnitFromName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/UnitName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/UnitName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/SpecName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/SpecName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/SpecName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/SpecFromName.cs Show resolved Hide resolved
Revit_Core_Engine/Query/SpecFromName.cs Show resolved Hide resolved
@vietle-bh
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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

Copy link
Contributor

@vietle-bh vietle-bh left a comment

Choose a reason for hiding this comment

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

Tested and it worked 👍

@pawelbaran
Copy link
Member Author

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

Copy link

bhombot-ci bot commented Aug 13, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check core
  • check serialisation
  • check null-handling

Copy link

bhombot-ci bot commented Aug 13, 2024

@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
Copy link
Member Author

@BHoMBot check installer

Copy link

bhombot-ci bot commented Aug 14, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check installer

There are 2 requests in the queue ahead of you.

@pawelbaran
Copy link
Member Author

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Aug 14, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check ready-to-merge

@pawelbaran pawelbaran merged commit 4ff3d21 into develop Aug 14, 2024
12 checks passed
@pawelbaran pawelbaran deleted the Revit_UI-#438-UnitsInFiltering branch August 14, 2024 09:33
@BHoMBot BHoMBot mentioned this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants