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

Format selection #2272

Merged
merged 59 commits into from
Jun 26, 2022
Merged

Format selection #2272

merged 59 commits into from
Jun 26, 2022

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented May 20, 2022

I'm trying to re-enable Format selection in Fantomas.
The old code used the tokenizer we won't don't expose anymore in Fantomas.FCS so I took a different approach.

The idea is to find an AST node (of a predefined selected set of types) and format it using an anonymous module. We exact the closest selected node and format from AST to get the result.
This isn't perfect but will work for 80 - 90% of the cases.

As a side remark, I'm strongly considering #2271 as well, but in a separate PR.

Initially thoughts @baronfel, @jindraivanek or @dawedawe?

TODO:

  • Feedback David
  • Add documentation to manage expectations
  • Format selection in signature files
  • Update Daemon
  • Sign off on public API

Copy link
Member

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

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

Yeah, this approach makes sense to me. Just some nitpicks in the comments.

src/Fantomas.Core.Tests/FormattingSelectionOnlyTests.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/Selection.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/Selection.fs Outdated Show resolved Hide resolved
src/Fantomas.Core/Selection.fs Outdated Show resolved Hide resolved
@nojaf
Copy link
Contributor Author

nojaf commented May 23, 2022

Thanks for taking a look @dawedawe!
@baronfel any thoughts on the return type of FormatSelection?
There are multiple things that could go wrong: invalid selection, unsupported AST node, a selection with compiler directives which might not end well and probably some other stuff. This is a rather complex thing to get right. The daemon could pass along some detailed info on what went wrong, not sure how far we want to go there. Thoughts?

@nojaf
Copy link
Contributor Author

nojaf commented May 23, 2022

Note for me: the restored trivia needs to be inside the selection. This will currently not be the case.

@nojaf nojaf force-pushed the format-selection branch from f0d8191 to 821c143 Compare June 22, 2022 08:58
@nojaf
Copy link
Contributor Author

nojaf commented Jun 23, 2022

Note to self: larger selections than the range of a node should not overlap with overlap other nodes.
For example: let myFunction() = (), le`` myFunction()`` = ().
Keep it simple, support exact ranges and ranges with leading and or trailing whitespaces.

@nojaf nojaf marked this pull request as ready for review June 25, 2022 10:11
@nojaf
Copy link
Contributor Author

nojaf commented Jun 25, 2022

Hello @dawedawe, this is a lot to review so I won't ask that you review every file.
However, could you please maybe take a look at formatSelection in Selection.fs?
Follow the code from there on and see if it still makes sense to you.
Maybe some places could use an extra comment. Just skim through it and let me know if something is unclear.

Copy link
Member

@dawedawe dawedawe left a comment

Choose a reason for hiding this comment

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

Seems very well structured to me.
I hope people appreciate how much work you put into this feature.

docs/Documentation.md Outdated Show resolved Hide resolved
src/Fantomas.Client/LSPFantomasService.fs Outdated Show resolved Hide resolved
src/Fantomas.Client/LSPFantomasService.fs Outdated Show resolved Hide resolved
@nojaf
Copy link
Contributor Author

nojaf commented Jun 26, 2022

Thanks for the review @dawedawe! I'm going to merge this one in, update the online tool and ask you to test this against your code base to detect any regressions.
I already did this against Fantomas, the F# compiler and FsAutocomplete so it should be fairly stable.
But there still could be some edge cases where a comment is lost.

@nojaf nojaf merged commit 4d98e26 into fsprojects:master Jun 26, 2022
@nojaf nojaf deleted the format-selection branch June 26, 2022 09:41
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.

Refactor list of TriviaNodeAssigner to tree structure Restore the FormatSelection public API
2 participants