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

Add support for editorconfig #940

Merged
merged 14 commits into from
Jul 6, 2020
Merged

Add support for editorconfig #940

merged 14 commits into from
Jul 6, 2020

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Jul 1, 2020

Current attempt for solving #650.
EditorConfig.Core was forked to https://github.com/nojaf/editorconfig-core-net.
There I created a slightly modified version, mainly having a netstandard2.0 only configuration.

I used ILRepack to merge in the editorconfig core to Fantomas.dll.
I published Fantomas.EditorConfig.Core as unlisted to Nuget.

I fear that this PR is already competing with #939, for that @josefblaha I'm sorry.
Last Friday I already had those changes to readConfiguration .

Main things left to do here are:

  • update unit tests
  • integrate into CLI tool
  • remove all traces of JSON config (including schema)

@nojaf nojaf linked an issue Jul 1, 2020 that may be closed by this pull request
@josefblaha
Copy link

What an exciting world is this OSS development, with developers racing to complete features ;-)

@nojaf, you are on a good track to finish this, so I will no purse it anymore. Just two things came to my mind when reviewing your code:

  1. For IndentSpaceNum option, there is no one-to-one mapping with editorconfig. IMO the value should be derived from indent_style, indent_size, and tab_width properties.
  2. No warnings should be generated from unrecognized properties; any other custom properties or possible future global properties should be ignored.

@josefblaha josefblaha mentioned this pull request Jul 2, 2020
3 tasks
@nojaf
Copy link
Contributor Author

nojaf commented Jul 2, 2020

Hey @josefblaha, again apologies for not creating my PR sooner. I should have made a draft PR last Friday, that may have avoided this.

As for your feedback:

  1. F# doesn't allow tabs so we only need to take spaces into account.
  2. Good point, warnings should not be listed anymore as they count have meaning to other things.

@josefblaha
Copy link

No problem. Any code written has a value for me, as I'm still learning the language.

Ad tabs. You're right, but there is a corner case: indent_size = tab means that you need to look on tab_width to get the value. See (https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#widely-supported-by-editors).

@nojaf
Copy link
Contributor Author

nojaf commented Jul 3, 2020

Actually, the underlying EditorConfig.Core Nuget solves this internally already,
If I had for example:

[*.fs]
indent_style=tab
indent_size=tab
tab_width=5
fsharp_indent_on_try_with=true

I got
image

I'll add a unit test to cover this as well.

@nojaf nojaf marked this pull request as ready for review July 3, 2020 09:22
@nojaf
Copy link
Contributor Author

nojaf commented Jul 3, 2020

This is ready for now.
A couple of significant changes:

  • You don't need to pass down the configuration file in the cmd tool. Fantomas will pick it up based on the original file you passed.
    One trade-off here is that you can't pass a configuration anymore when you use the stdIn. I would add this back in if requested.
    I'm not sure if many people actually use this.
  • Same remark for using the FAKE Helpers, config will be picked up based on the F# file, to me this is the way to go.
    I'll update the sample once there is a new alpha released.
  • I've renamed IndentSpaceNum and PageWidth to match the .editorconfig names.
  • Fantomas-tools would need an update after this PR.

@nojaf nojaf merged commit 07fb56b into fsprojects:master Jul 6, 2020
@nojaf nojaf deleted the fix-650 branch December 11, 2020 13:27
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.

Use .editorconfig to define Fantomas settings
3 participants