-
Notifications
You must be signed in to change notification settings - Fork 786
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
[FS-1080] Add new syntax option for Float32. #7839
[FS-1080] Add new syntax option for Float32. #7839
Conversation
Thanks for the trial implementation @gdziadkiewicz - I've tagged this as |
tests/fsharp/Compiler/Conformance/BasicGrammarElements/BasicConstants.fs
Outdated
Show resolved
Hide resolved
f3f7684
to
3cbd467
Compare
@TIHan @dsyme @KevinRansom can you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation is great.
The one thing missing is locking the feature to a language version and having tests cover that. This is partly our fault as we need better ways to handle language version in our tests. I have to do this anyway with the 'default interface method consumption' feature, so I may work a lot on tests this week.
@TIHan Then I will wait for your work to copy the approach. |
Agreed with @TIHan - just needs the language version implementation |
This is nice, lang version might be a bit tricky. |
Okay Lexbuffer<'Char> has supportsLanguageVersion: https://github.com/dotnet/fsharp/blob/release/fsharp5/src/utils/prim-lexing.fsi#L112 And it's available right here: Line 338 in cad335e
So I guess not so tricjky, |
ccc7046
to
4e8c234
Compare
@TIHan Can you check my approach to the language version? |
@KevinRansom can also take a look, he did the fun work of backporting LangVersion into various merged features for F# 4.7 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, some minor quibbles
1ec8bf3
to
ef4a26a
Compare
# Conflicts: # src/fsharp/LanguageFeatures.fs
@cartermp Is there anything more that I should do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good @gdziadkiewicz, thanks! We'll need review from @dsyme to see this merged, though.
LGTM |
@gdziadkiewicz looks like there's some conflicts, but directly fixing them may cause a build break. Could you take care of that? After that's done we'll merge this in. Great work, and thanks for the patience! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I'm going to move it to fsharp5 with all of the other fsharp5 language features. However, that is all coming back to master shortly.
@gdziadkiewicz, the link in the original post gives a 404, was it moved? Incorrect: https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1080-float32-without-dot.md. |
@Happypig375, thanks. I've reported the underlying issue that we ought to expect all RFCs to be in one place, or be redirected if moved. |
Suggestion: fsharp/fslang-suggestions#750
RFC: https://github.com/fsharp/fslang-design/blob/master/FSharp-5.0/FS-1080-float32-without-dot.md