-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversions of char->decimal, string->nativeint, and string->unativeint #1040
Comments
I'm going to decline this one For For |
@dsyme But FSharp.Core already includes nativeint and unativeint parsers when ExplicitDynamic is called directly. |
@dsyme char->decimal is not ok but decimal->char is fine? |
Other floating-points are also ok? |
Yes, you're correct (for others - they parse to int64 and convert using overflow checking) OK I guess these are ok
Hmmm. I can see the argument from consistency. Though I don't like either of them TBH. The intention is F# was never really to treat I don't really like admitting more of these, but the argument from consistency wins I guess. However I don't think anyone seriously wants to go from I'll reopen and approve. |
Current behaviours of the other functions are also fine. No need to support them for user code and it's not the responsibility of F# to provide these things. |
Oh so deprecate
Just because you don't need them is not a reason to deny others from having this option. |
I personally agree from an OCaml/ML/how-F#-wants-programming-to-be perspective. However there is an argument from a different perspective - the System.Decimal is a metadata-defined type, and if we looks at the op_Implicit and op_Explicit for System.Decimal they allow explicit Now F# only treats System.Decimal in a bespoke way, because it allows unit-of-measure on the type, and gives unitized perspective on addition etc. This is one reason inconsistencies crept in - we intercept constraint solving for System.Decimal, but do it imperfectly.
I think in this case deprecation "against the will of the .NET metadata" will only lead us to more inconsistencies. For example we'd need to carefully consider if the .NET metadata conversions would still apply via dotnet/fsharp#10884. (This is a general concern with dotnet/fsharp#10884, that last-resort TDCs via op_Implicit might creep in even for quite foundational types, or metadata-defined foundational types like System.Half. I don't really see how we can avoid that.)
I think that, given we provide the facility
FWIW this was not the argument being made, which was more about what FSharp.Core should and shouldn't do. As an aside, let's try to avoid personalising things, and avoid attributing thoughts to others, e.g. try avoid words like "you". I greatly enjoy the differering perspectives people bring to this repo. Let's keep the discussions enjoyable and fun, based on the set of design principles outlined in the notes in this repo - or similar recognised design principles 🙂 🙏 🙂 |
To summarise re
Given that I think it's right for consistency to include decimal in the matrix. For
Given that I think it's right to now include these in the matrix. |
This can be closed as it has been completed here dotnet/fsharp#11681 by @Happypig375 |
Conversions of char->decimal, string->nativeint, and string->unativeint
I propose we stop being inconsistent and add these 3 conversions.
The existing way of approaching this problem in F# is
#nowarn "1204"
, or having to write conversion functions by ourselves.Pros and Cons
The advantages of making this adjustment to F# are
The disadvantages of making this adjustment to F# are that (insert the reason for specifically excluding these 3 conversions as written in https://github.com/dotnet/fsharp/blob/954210d1e8590b708801b2e9f70717f09979b9c0/src/fsharp/ConstraintSolver.fs#L1448-L1451).
Extra information
Estimated cost (XS, S, M, L, XL, XXL): XS
Related suggestions: #1030 - Subtraction of two chars
Affidavit (please submit!)
Please tick this by placing a cross in the box:
Please tick all that apply:
For Readers
If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.
The text was updated successfully, but these errors were encountered: