fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Add new Units type to ease UoM conversions

Open roboz0r opened this issue 1 year ago • 7 comments

Description

Creates a new Units type to meet some of the goals in https://github.com/fsharp/fslang-suggestions/issues/892

  • https://github.com/fsharp/fslang-design/pull/784

The type adds Units.Add, Units.Remove, and Units.Cast methods for supported primitive types to simplify UoM conversions. It also adds Units.Add*, Units.Remove*, and Units.Cast* e.g. Units.AddArray to perform UoM conversions on common collection types without allocations.

I haven't added any tests as the code doesn't actually do anything, it's just a wrapper over retype. Happy to add anything you think would be helpful.

Checklist

  • [ ] Test cases added
  • [ ] Performance benchmarks added in case of performance changes
  • [x] Release notes entry updated

roboz0r avatar Aug 10 '24 22:08 roboz0r

:heavy_exclamation_mark: Release notes required


:white_check_mark: Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/9.0.300.md

github-actions[bot] avatar Aug 10 '24 22:08 github-actions[bot]

Few questions:

  1. What bothers me is that this is not how we typically add functions to fsharp core - a type with static methods with a bunch of overloads
  2. I think it contradicts a bit with what's approved in the suggestion - what functions and in which part of corelib.
  3. It is missing a bunch of tests and documentation
  4. What's the difference in assembly size before and after the addition?

I think before the implementation we need to start with RFC, and once that is approved and discussed, we can proceed with implementation

vzarytovskii avatar Aug 11 '24 07:08 vzarytovskii

  1. Agree it's not typical for fsharp core code but I found the ergonomics and discoverability much nicer to have 15 (primitive + 4 collections * 3) methods with 13 overloads than having 195 differently named methods.
  • The naming and overall API are open to discussion but I felt it better to put out a concrete implementation to serve as a starting point for discussion.
  1. The API design was inspired by https://github.com/fsharp/fslang-suggestions/issues/892#issuecomment-1191993569 the most recent comment in the that was received broadly positively from the few thumbs up. Agree that it is somewhat inconsistent with the initial description but I tried to synthesize with all the comments while avoiding the generic conversion which would require a language change.
  • If you feel it would be better placed in LanguagePrimitives then that's an easy change to make, though as a noob I always felt the LanguagePrimitives module was intended as compiler internal helpers
  1. Definitely still need to add the documentation. I intentionally held off on this, anticipating that the API specific were likely to change.
  • What did you have in mind for the tests? I don't think there's any logic to test so would it just be a series of tests with explicit type annotations and if they compile then it's good?
  1. Comparing the resulting size of FSharp.Core.dll (built with ./Build.cmd -c Release) is 2,250KB to 2,281KB

roboz0r avatar Aug 11 '24 14:08 roboz0r

I think we should still start with rfc for it, so everyone agrees on specifics.

vzarytovskii avatar Aug 11 '24 16:08 vzarytovskii

I put together a draft RFC here https://github.com/fsharp/fslang-design/pull/784 is there something else I missed?

roboz0r avatar Aug 11 '24 16:08 roboz0r

I put together a draft RFC here https://github.com/fsharp/fslang-design/pull/784 is there something else I missed?

That looks like a good start, now we need some comments on it. I'll probably be also good to have all types next to examples so people get a good understanding how many methods/functions are added

vzarytovskii avatar Aug 11 '24 17:08 vzarytovskii

@roboz0r : We have dicussed this addition and have an idea on how to bring the same functionality with a new compiler intrinsic, instead of adding many new functions to FSharp.Core. Vlad will ping you on Discord on how this could work.

This would especially mean a lot smaller size footprint.

T-Gro avatar Aug 14 '24 17:08 T-Gro

(coverting to draft due to the suggestions above)

T-Gro avatar Dec 03 '24 16:12 T-Gro