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

[Python] Use Fantomas formatting and check CI #2738

Merged
merged 5 commits into from
Jan 17, 2022

Conversation

dbrattli
Copy link
Collaborator

@dbrattli dbrattli commented Jan 16, 2022

So this PR might be a bit controversial with Fable 😬 but I think it's mostly a good thing with a common code style when we have many contributors. So starting with Python related code we will now enforce the formatting based on the latest Fantomas 4.6 (set as a dotnet tool) and the rules set in .editorconfig. I'm pretty agnostic to what the rules should be as long as we have a set of common rules. The current .editorconfig tries to match the current code style, but please make PR's to modify if to the Fable style.

This PR will only enforce the formatting of src/Fable.Transforms/Python but we should consider adding this to the rest of the code base as well.

To format the Python code, you will need to run:

> dotnet tool restore
> dotnet fantomas src/Fable.Transforms/Python

Fantomas is added to GitHub actions so if the Python related code is not formatted according to the rules, the build will fail. Please chime in if you do not want this kind of restrictions in Fable (even if only applied to the Python specific code) @alfonsogarciacaro @ncave

@ncave
Copy link
Collaborator

ncave commented Jan 16, 2022

@dbrattli Some of the code base in Fable comes from dotnet/fsharp where there isn't any formatting, not even EOL white-space removal. So yes, IMO it would be preferable if we keep the original formatting and not enforce Fantomas on everything.

@dbrattli
Copy link
Collaborator Author

@ncave It's possible to ignore parts of the code-base using .fantomasignore, so copied / auto-generated code can be ignored from formatting. https://github.com/fsprojects/fantomas/blob/master/docs/Documentation.md#ignore-files-fantomasignore


let path =
match relative, normalizedPath with
| _, "" -> ""
| true, "." -> "."
| true, path ->
path // translate path to Python relative syntax
path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantomas has removed the comment here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've seen this happen a couple of times. Added issue at: fsprojects/fantomas#2027

@alfonsogarciacaro
Copy link
Member

Yes, I've been thinking about adding Fantomas many times, but at the end I discarded by issues like the one pointed out by @ncave. Particularly right now it can make keeping main in beyond in sync more difficult, so I would postpone the decision at least until beyond replaces main. It should be fine to apply Fantomas to Fable.Transforms/Python though.

@dbrattli
Copy link
Collaborator Author

Ok, thanks for the comments. It makes sense to wait with this until after beyond merges with main. I'll still add this for Python to test if it can work, but will remove the CI hook if it becomes to annoying for the rest of you.

.editorconfig Outdated Show resolved Hide resolved
@dbrattli dbrattli merged commit f6c58f1 into fable-compiler:beyond Jan 17, 2022
@dbrattli dbrattli deleted the python-fantomas branch January 17, 2022 18: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.

4 participants