Skip to content

Conversation

@jindraivanek
Copy link
Contributor

@jindraivanek jindraivanek commented May 10, 2022

Fix #2198.

Introduce new config option KeepMaxBlankLines that set maximal number of consecutive blank lines from source code that is kept in formatted code.

I'm open to suggestion to better name for new option.

Option is now implemented as a number, with big value as default. Is it OK, or it should be Num option?

KeepMaxBlankLines=0 reveals multiple cases where Fantomas should add blank lines - I think this should be solved as separate issue(s).

Checklist:

  • agree on name of new option
  • new option: Num or Num option?
  • ignored tests with issues for KeepMaxBlankLines=0

@jindraivanek jindraivanek requested a review from nojaf May 10, 2022 20:15
Copy link
Contributor

@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.

This looks good! I've added one remark in terms of creating the Newline trivia, that might be an interesting one to consider. Otherwise, a couple of nits but nothing serious.

I would also like to see the edge cases here. I wasn't able to see the limitations that fsharp_keep_max_blank_lines=0 has.
We could have this as an ignored test to highlight we are at least aware of when this will do weird things.

Well done!

@jindraivanek
Copy link
Contributor Author

I would also like to see the edge cases here. I wasn't able to see the limitations that fsharp_keep_max_blank_lines=0 has.
We could have this as an ignored test to highlight we are at least aware of when this will do weird things.

It can be seen from my experiment here #2198 (comment) (it was POC, but result should be same - I try to reproduce same thing with this implementation later).

I will create ignored test highlighting problematic case from this.

@nojaf
Copy link
Contributor

nojaf commented May 11, 2022

but result should be same

I'm not entirely sure about that, because this time, less trivia was assigned to a trivia node and I believe in the original POC the newlines were swallowed in CodePrinter.
So I'm not sure if you can still have the weird behaviour.

@jindraivanek jindraivanek requested a review from nojaf May 11, 2022 20:37
@jindraivanek
Copy link
Contributor Author

I made requested changes.

Fantomas.Core formatted with fsharp_keep_max_number_of_blank_lines=0 is here: jindraivanek@ece950f

Only thing missing is ignored test(s) with problematic cases from there.

Copy link
Contributor

@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.

Spot on! Thank you.

@nojaf nojaf merged commit 91e480e into fsprojects:master May 12, 2022
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.

Reduce multiple blank lines to a single one.

2 participants