-
Notifications
You must be signed in to change notification settings - Fork 796
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
RFC FS-1108 - Allow more undentations and remove inconsistencies #11772
Conversation
@auduchinok I haven't included fsharp/fslang-suggestions#514 in here, which your case belongs to. Not sure if I can fix attribute indentation just as easily with the others. |
I can't run FSharpQA locally because Perl is complaining about Win32::Process not being installed and there is no instructions on how to deal with this error, while trying to use StrawberryPerl's bundled cpanm to install Win32::Process tells me that ExtUtils::Manifest must be installed first, and it cannot be installed because I need... ExtUtils::Manifest? I can't resolve this circular reference. |
@Happypig375 I'll go through the suggestions and check the approvals. Best to keep to the approved suggestions to avoid having to factor out the unapproved ones? |
@dsyme As written in the draft RFC, this tries to be a general fix to a lot of suggestions with the same underlying cause. Implementing the suggestions one by one by introducing special cases after special cases just introduce more inconsistency. |
The changes look plausible - please keep them minimal as we would want to check this very carefully The RFC spec also looks plausible, I'll comment there seperately |
I've played around with this and was able to verify all my samples that used to produce warnings. |
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.
Marking as approved, as just two minor test cases need to be added (though I think they may actually already be under test)
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.
Some changes are needed, I've identified a failing scenario
@Happypig375 I put this
in
and the compilation fails
The problem can be reduced to this: begin match 1 with
| 1 -> ()
| 2 ->
f()
end Thanks (It's possible we'd think that this shouldn't be allowed, but it is) |
@Happypig375 Can you fix this one? #11772 (comment) thanks |
Will come back a few days later, I'm busy now |
I'm back |
@dsyme Fixed |
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.
Thank you for addressing the last remaining items
Thanks for this work, will this produce a warning for this quirk? Given this this operator for evaluation of side effects in pipelines let inline ( |>! ) a f = f a ; a and this code snippet if 1 = 1 then
"same"
else
"different"
|>! printfn "The numbers are %s."
|> printfn "Yes, they are %s." This never prints |
This PR does not add additional warnings. |
Thanks for clarification @Happypig375, I guess my desired warning is tracked in fsharp/fslang-suggestions#806 |
RFC Allow more undentations and remove inconsistencies