Skip to content
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

Merged
merged 28 commits into from
Aug 9, 2021

Conversation

Happypig375
Copy link
Member

@Happypig375 Happypig375 commented Jul 2, 2021

@Happypig375 Happypig375 marked this pull request as draft July 2, 2021 13:04
@auduchinok
Copy link
Member

Here's another case, if it can be added. 🙂

Screenshot 2021-07-02 at 18 51 28

@Happypig375
Copy link
Member Author

@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.

@Happypig375 Happypig375 closed this Jul 4, 2021
@Happypig375 Happypig375 reopened this Jul 4, 2021
@Happypig375
Copy link
Member Author

Happypig375 commented Jul 4, 2021

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.

@dsyme
Copy link
Contributor

dsyme commented Jul 5, 2021

@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?

@Happypig375
Copy link
Member Author

@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.

src/fsharp/LexFilter.fs Outdated Show resolved Hide resolved
src/fsharp/LexFilter.fs Show resolved Hide resolved
src/fsharp/LexFilter.fs Outdated Show resolved Hide resolved
src/fsharp/LexFilter.fs Outdated Show resolved Hide resolved
src/fsharp/LexFilter.fs Show resolved Hide resolved
@dsyme
Copy link
Contributor

dsyme commented Jul 6, 2021

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

@nojaf
Copy link
Contributor

nojaf commented Jul 28, 2021

I've played around with this and was able to verify all my samples that used to produce warnings.
It all worked fine and I'm looking forward to this. Many thanks again @Happypig375 🎉!

Copy link
Contributor

@dsyme dsyme left a 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)

Copy link
Contributor

@dsyme dsyme left a 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

@dsyme
Copy link
Contributor

dsyme commented Jul 28, 2021

@Happypig375 I put this

<OtherFlags>$(OtherFlags) --langversion:preview</OtherFlags>

in

src/fsharp/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj 

and the compilation fails

C:\GitHub\dsyme\fsharp\src\fsharp\absil\ilwritepdb.fs(611,15): error FS0058: Possible incorrect indent
ation: this token is offside of context started at position (608:17). Try indenting this token further
 or using standard formatting conventions. [C:\GitHub\dsyme\fsharp\src\fsharp\FSharp.Compiler.Service\
FSharp.Compiler.Service.fsproj]

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)

@dsyme
Copy link
Contributor

dsyme commented Aug 1, 2021

@Happypig375 Can you fix this one? #11772 (comment) thanks

@Happypig375
Copy link
Member Author

Will come back a few days later, I'm busy now

@Happypig375
Copy link
Member Author

I'm back

@Happypig375
Copy link
Member Author

@dsyme Fixed

Copy link
Contributor

@dsyme dsyme left a 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

@dsyme dsyme merged commit f767719 into dotnet:main Aug 9, 2021
@goswinr
Copy link
Contributor

goswinr commented Aug 11, 2021

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 The numbers are same but it does print Yes, they are same. (Also asked on SO)

@Happypig375
Copy link
Member Author

This PR does not add additional warnings.

@goswinr
Copy link
Contributor

goswinr commented Aug 11, 2021

Thanks for clarification @Happypig375, I guess my desired warning is tracked in fsharp/fslang-suggestions#806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants