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

Turn on strict indentation. #3014

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Turn on strict indentation. #3014

merged 2 commits into from
Dec 11, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 11, 2023

As expected, the strict indentation in the Fantomas.FCS parser was not enabled.
I don't think it matters much for Fantomas, as we expect a valid tree anyway.
This could be useful for the online tool though.

//cc @auduchinok

@nojaf nojaf requested a review from dawedawe December 11, 2023 07:43
@auduchinok
Copy link
Contributor

Thanks @nojaf!

I've also thought there's a chance we could want to have a setting for it somewhere in the online tool one day, but I don't think it's worth it much to have it in advance 🙂

@auduchinok
Copy link
Contributor

I've thought a bit more and now I'm a bit worried that Fantomas doesn't know about the language version that is used in a project/SDK. It may start to parse things differently and produce wrong results on code that wasn't updated for the strict indentation and has indentation warnings. At the same time not having this setting enabled would work correctly for both updated (without these warnings) and not updated code. Maybe we should not turn it on in Fantomas yet... But I'd be happy use this mode in the online tool at least optionally.

@auduchinok
Copy link
Contributor

auduchinok commented Dec 11, 2023

I don't think it matters much for Fantomas, as we expect a valid tree anyway.

Without this setting some code would have warnings about the indentation, but not errors. Does it count as a valid tree?

I've just tried on example in the online tool, and it seems the strict indentation is already enabled.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 11, 2023

Interestingly enough, this change in the PR does not affect the parse error of

let x =
1

We need to pass in Some false in order to opt-out of strict indentation.
So None and Some true seem to do the same thing. Which is not really what I expected.

I've thought a bit more and now I'm a bit worried that Fantomas doesn't know about the language version that is used in a project/SDK.

Nope, Fantomas assumes preview and doesn't do language versions.
This is a very reasonable tradeoff in terms of maintenance.

@nojaf nojaf merged commit d724a64 into fsprojects:main Dec 11, 2023
5 checks passed
@auduchinok
Copy link
Contributor

We need to pass in Some false in order to opt-out of strict indentation.
So None and Some true seem to do the same thing. Which is not really what I expected.

None just makes it use the defaults, and Some value allows you to override it with the value. With a newer compiler the defaults are to use the strict indentation. This setting is only expected to be used temporarily, we'll hopefully remove it in future.

@nojaf nojaf deleted the strict-indentation branch December 11, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants