Removing nullable methods from quantity classes as well as extension … #483
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
angularsen
requested changes
Sep 24, 2018
| /// <inheritdoc cref="AmountOfSubstance.FromMillimoles(UnitsNet.QuantityValue)" /> | ||
| public static AmountOfSubstance Millimoles(this float value) => AmountOfSubstance.FromMillimoles(value); | ||
|
|
||
| /// <inheritdoc cref="AmountOfSubstance.FromMillimoles(UnitsNet.QuantityValue)" /> | ||
| public static AmountOfSubstance? Millimoles(this float? value) => AmountOfSubstance.FromMillimoles(value); | ||
|
|
||
| /// <inheritdoc cref="AmountOfSubstance.FromMillimoles(UnitsNet.QuantityValue)" /> | ||
| public static AmountOfSubstance Millimoles(this decimal value) => AmountOfSubstance.FromMillimoles(Convert.ToDouble(value)); |
There was a problem hiding this comment.
I think we can remove decimal too, since the main scenarios I can think of are:
1.Meters();
1.5.Meters();
1.5m.Meters(); // For decimal, probably not so common
decimal myVal = 1.5m;
myVal.Meters(); // Also I'm guessing not common, I would not use it this way at least
angularsen
requested changes
Sep 24, 2018
| @@ -49,135 +49,75 @@ public static class NumberToAmplitudeRatioExtensions | |||
| /// <inheritdoc cref="AmplitudeRatio.FromDecibelMicrovolts(UnitsNet.QuantityValue)" /> | |||
| public static AmplitudeRatio DecibelMicrovolts(this int value) => AmplitudeRatio.FromDecibelMicrovolts(value); | |||
|
|
|||
| /// <inheritdoc cref="AmplitudeRatio.FromDecibelMicrovolts(UnitsNet.QuantityValue)" /> | |||
| public static AmplitudeRatio? DecibelMicrovolts(this int? value) => AmplitudeRatio.FromDecibelMicrovolts(value); | |||
|
|
|||
| /// <inheritdoc cref="AmplitudeRatio.FromDecibelMicrovolts(UnitsNet.QuantityValue)" /> | |||
| public static AmplitudeRatio DecibelMicrovolts(this long value) => AmplitudeRatio.FromDecibelMicrovolts(value); | |||
| /// <inheritdoc cref="AmplitudeRatio.FromDecibelMicrovolts(UnitsNet.QuantityValue)" /> | ||
| public static AmplitudeRatio DecibelMicrovolts(this double value) => AmplitudeRatio.FromDecibelMicrovolts(value); | ||
|
|
||
| /// <inheritdoc cref="AmplitudeRatio.FromDecibelMicrovolts(UnitsNet.QuantityValue)" /> | ||
| public static AmplitudeRatio? DecibelMicrovolts(this double? value) => AmplitudeRatio.FromDecibelMicrovolts(value); | ||
|
|
||
| /// <inheritdoc cref="AmplitudeRatio.FromDecibelMicrovolts(UnitsNet.QuantityValue)" /> | ||
| public static AmplitudeRatio DecibelMicrovolts(this float value) => AmplitudeRatio.FromDecibelMicrovolts(value); |
|
Good call about removing nullable From methods, twice the method count for a minor syntax convenience. |
|
Did one better... Rather than potentially breaking existing code, I changed the extension methods to be generic. This should allow any type that can be called with Convert.ToDouble to be used. This actually expands the available use cases while also reducing the size of the dll by another 150 KB! Total size is now 756 KB. We're basically to half the size we were before, plus it will scale with additional units much better. |
|
Awesome! I'm so puzzled, I was so sure I had already tried the generics route and it failed, but a quick prototype just now confirms it works. Very nice! Soo, we have two ways to go about it then. Add back the nullable From methods and merge this into master now, or merge this into a v4 kind of branch and start getting a breaking change release going before merging any of it back to master. |
|
I created PR #485 to use generics without the major API changes of removing the nullable methods. This can be merged immediately. We can handle this PR however we decide for the next major version. |
tmilnthorp
force-pushed
the
RemoveNullableMethods
branch
from
5aaf777 to
f59e55a
Compare
Merged
angularsen
added a commit
that referenced
this pull request
Dec 16, 2018
# 4.0.0 Release This PR will serve as the list of items to complete and it will be updated to show the progress before finally merged into `master` when completed. We all have busy schedules, so **if you want to help move this work forward then that is much appreciated!** ## The main theme is to reduce binary size In two years it has grown from 280 kB to 1.4 MB and a lot of it is due to unnecessary syntactic sugar with many method overloads for various number types and nullable types - for every of our 800+ units! It simply adds up to a big total. These items are chosen from #180, trying to include as many of the low hanging fruits as possible, while still keeping the list short and realistic to complete in a reasonably short time - we are all busy in our daily lives. We can always have more major version bumps later than trying to perfect it all now. ## Feature complete before November, merged before mid-December I would like to aim for a "beta" pre-release nuget sometime in October with all the items completed, so we can have some time to test it before releasing the final, stable 4.0.0 version before Christmas holidays. We should have the work items list more or less final before Monday, October 8th. ## Changes #### Added - [x] UnitSystem with a different meaning, now defines the base units for a unit system (#524) #### Removed - [x] Replace netstandard1.0 target with netstandard2.0, to avoid the extra dependencies (#477) - [x] Remove code marked as `[Obsolete]`, such as `VolumeUnit.Teaspoon` (#490) - [x] Remove nullable `From` factory methods (#483) - [x] Remove extension methods on _nullable_ number types (#483) - [x] Remove all number extension methods (#497) - [x] Remove `Length2d`, replaced by `Area` (#501) - [x] Remove static methods on `UnitSystem`, should use `UnitSystem.Default` instead (#496) - [x] Remove unit parameter from `ToString()` methods (#546) #### Changed - [x] Throw exception on NaN values in static constructor methods, like `FromMeters()` (#502, see #176 (comment)) - [x] Throw if unit was not specified when constructing quantities, (#499 - #389 (comment)) - [x] Stricter parsing (#343 and #180 (comment)) - [x] Remove unit parameter from `ToString()` methods (#546) - [x] Change Temperature arithmetic back to use base unit Kelvin (#550, see #518) #### Renamed - [x] Correct SingularName for some Flow unit definitions (#494, see #360) - [x] Split/rename UnitSystem into UnitParser, GlobalConfiguration, UnitAbbreviationsCache (#511) #### Fixed - [x] Search for any TODO comments in the code to address, remove comment and either fix or create issue (5d24432) - [x] Update README with v4 changes #498 - [x] Do not try/catch in UnitConverter.Try-methods (#506) - [x] Do not try/catch in `Length.TryParse()` and for other quantities (#507) - [x] Add/update changelog in wiki for v4 with some summary copied from this issue, some v3 to v4 migration instructions and briefly explain why some stuff were removed ## Milestones - [x] Finalize work items list (Monday, October 8) - [x] Release 4.0.0-beta1, feature complete (Wednesday, October 31) - [ ] ~Show release notes and upgrade guide when upgrading to 4.x nuget, if possible~ - [x] Release 4.0.0 (Monday, December 17)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removing nullable methods from quantity classes as well as extension methods.
Work towards reducing library size #372