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

🔥Remove static Default getters, obsoleted by UnitsNetSetup #1475

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

angularsen
Copy link
Owner

@angularsen angularsen commented Dec 27, 2024

Ref #1200

Remove static Default getter property for UnitConverter, UnitAbbreviationsCache, QuantityParser, UnitParser. These just pointed to the singleton UnitsNetSetup.Default and were made obsolete in v5.

@angularsen angularsen mentioned this pull request Dec 27, 2024
20 tasks
@angularsen angularsen merged commit a3c32b6 into release/v6 Dec 27, 2024
1 check was pending
@angularsen angularsen deleted the agl/rem-default branch December 27, 2024 23:00
Copy link

codecov bot commented Dec 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86%. Comparing base (104c67a) to head (ce11086).
Report is 1 commits behind head on release/v6.

Additional details and impacted files
@@            Coverage Diff             @@
##           release/v6   #1475   +/-   ##
==========================================
  Coverage          86%     86%           
==========================================
  Files             298     298           
  Lines           30520   30516    -4     
==========================================
  Hits            26404   26404           
+ Misses           4116    4112    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lipchev
Copy link
Collaborator

lipchev commented Dec 28, 2024

@angularsen Sorry, I've just gotten around to review these recent changes.. I know these had been made [Obsolete] for a while now, but are you sure there is sufficient reason to remove them?

  1. As long as the dependency between the types is correct (it is) then there shouldn't be any harm in having them (do you see something I don't?)
  2. Even if we instantiate and use the concrete type, it's Default property isn't going to trigger the static constructor on the UnitsNetSetup, unless explicitly accessed
  3. I think it's pretty clear that these are simply shortcuts to UnitsNetSetup.Default.XXX - I personally find UnitParser.Default.Parse(..) to be much more intuitive than UnitsNetSetup.Default.UnitParser.Parse(..)

@angularsen
Copy link
Owner Author

angularsen commented Dec 28, 2024

I had the same thought, I don't have a strong opinion here. My thinking was that it may be better to have a single way to access these things, for docs/sample code and what not, but the syntax becomes more clunky.

The main benefit of moving things into UnitsNetSetup was ensuring the same instance of UnitAbbreviationCache was used, but there is no harm in having these getter properties as convenience.

I'm fine reverting this, but we should then remove the obsolete flags in v5 too.

@angularsen
Copy link
Owner Author

Sorry for not waiting for review and rushing a bit, but I have a little bit of extra time in the holidays and just want to get things done 😄 We can always revert, I'm glad to have a pair of extra eyes on these things 🙇

@lipchev
Copy link
Collaborator

lipchev commented Dec 28, 2024

Well, it was on the wish-list- I hadn't gotten around to arguing against it :)

I wasn't 100% sure whether they are safe in v5 but I was kinda hoping to remove the [Obsolete] tag in v6 (where I'm pretty sure it's all good).

Plus I'm sure there are many holdouts who ignored the [Obsolete] flag :)

@lipchev
Copy link
Collaborator

lipchev commented Dec 28, 2024

Plus I'm sure there are many holdouts who ignored the [Obsolete] flag :)

Here's one 🤣

angularsen added a commit that referenced this pull request Dec 29, 2024
…1475)"

This reverts commit a3c32b6.

- Add back default getters
- Remove obsolete message, rewrite xmldoc a bit
angularsen added a commit that referenced this pull request Dec 29, 2024
…1480)

Reverts #1475

This reverts commit a3c32b6.

- Add back default getters
- Remove obsolete message, rewrite xmldoc a bit
angularsen added a commit that referenced this pull request Dec 29, 2024
Per discussion in v6, we no longer want to remove these.

#1475 (comment)
angularsen added a commit that referenced this pull request Dec 29, 2024
Per [discussion
](#1475 (comment))
in v6, we no longer want to remove these.

Already reverted in v6 with #1480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants