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

style: try make code more consistent and align HTML DSL to 4 spaces indentation #948

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

MangelMaxime
Copy link
Contributor

@MangelMaxime MangelMaxime commented Dec 30, 2024

Like requested in #932 this is a PR dedicated to the Fantomas configuration.

When working on a new structure for the API documentation, I discovered that the current setup make it difficult to work with the HTML dsl.

Indeed, the elements inside of the list of the DSL are not naturally aligned to the standard 4 spaces indentations so you often need to manually place the code by adding 1, 2, 3 spaces, etc.

The important rules are to avoid the indentation issue are:

fsharp_experimental_elmish = true
fsharp_array_or_list_multiline_formatter = number_of_items
fsharp_max_array_or_list_number_of_items = 0
fsharp_multi_line_lambda_closing_newline = true
fsharp_multiline_bracket_style = aligned

The rest of the rules are here to enforce more consistency overall but are probably more subject to debate. I just wanted to take this opportunity of updating the Fantomas config to open the discussion.

For this PR, I applied the new rules to GenerateHtml.fs only but we could and perhaps should apply them to all the files for consistency ?

Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

I'd like a subset of these changes that focus on the DSL.
Otherwise it is just very inconsistent toward the rest of the codebase.

.editorconfig Outdated
fsharp_experimental_elmish = true
fsharp_array_or_list_multiline_formatter = number_of_items
fsharp_max_array_or_list_number_of_items = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really like this one, it makes too many single item list format in three lines. Makes it all rather excessive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes the code more consistent it always align to the 4 spaces guides and avoid having to reformat the list over and over when adding a new element:

CleanShot 2025-01-02 at 11 10 52@2x

For example, if I want to add a new element to embed with that configuration I can simply press Enter and be ready with it. Without I, need to move the list, align it, split the inner value on multiple lines, etc. Or add ; to add the new item but if it is too long I need to go back manually placing stuff around.

Copy link
Collaborator

Choose a reason for hiding this comment

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

pressing ; and formatting isn't the end of the world.
How many times are really facing this issue? If you had this once I really don't feel like this is worth it. If you have this 20 times in an hour I could kinda get it, but even then...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More often than I want to have to deal with it 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this code also make it harder to read some line IHMO.

CleanShot 2025-01-02 at 20 14 26@2x

I removed it but we can re-introduce if wanted before merging.

.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
@MangelMaxime
Copy link
Contributor Author

@nojaf Thanks for the review, I made a first pass to remove the rules which had no effect in this file.

Waiting for feedback on the rest, as I explained why the rule exist or not. Will adapt the PR accordingly :)

.editorconfig Outdated
@@ -3,10 +3,7 @@ root = true

[*]
end_of_line = lf

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't remove [*.{fs,fsi,fsx}], honestly rule of thumb change as little as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These settings make sense for all the files but ok

.editorconfig Outdated
fsharp_experimental_elmish = true
fsharp_array_or_list_multiline_formatter = number_of_items
fsharp_max_array_or_list_number_of_items = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

pressing ; and formatting isn't the end of the world.
How many times are really facing this issue? If you had this once I really don't feel like this is worth it. If you have this 20 times in an hour I could kinda get it, but even then...

.editorconfig Outdated Show resolved Hide resolved
@MangelMaxime
Copy link
Contributor Author

@nojaf I think I made all the changes requested.

Last ones that could change perhaps are:

If no more changes required this PR should be ready.

Copy link
Collaborator

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Thanks!

@nojaf nojaf merged commit b25094e into fsprojects:main Jan 3, 2025
3 checks passed
@MangelMaxime MangelMaxime deleted the feature/formatting_changes branch January 3, 2025 08:01
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.

2 participants