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 --consecutive option #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

phadej
Copy link

@phadej phadej commented Mar 7, 2025

Resolves #68

Add an option to check for maximum count of consecutive empty lines. If zero, unlimited cmount is allowed. If 1, there cannot be empty lines (1 empty lines triggers an error) , if 2 there can be one empty line (2 consecutive empty lines trigger an error), and so on.

I fail with naming, having value 1 to disallow any empty lines makes sense, but it's not "maximum amount of empty lines", it's more of "fail if there is that much empty lines".

Resolves agda#68

Add an option to check for maximum count of consecutive empty lines.
If zero, unlimited cmount is allowed. If 1, there cannot be empty lines,
if 2 there can be one empty line, and so on.

I fail with naming, having value 1 to disallow any empty lines makes
sense, but it's not "maximum amount of empty lines",
it's more of "fail if there is that much empty lines".
@phadej phadej force-pushed the consecutive-empty-lines branch from fc06231 to a7bf22a Compare March 7, 2025 17:48
@phadej
Copy link
Author

phadej commented Mar 7, 2025

Correction, -n 1 allows to have one empty line. So there is no way to disallow empty lines all-together. Having a golden test-suite to actually see how things are fixed would helped.

I can be changed either way.

@ulysses4ever
Copy link
Contributor

ulysses4ever commented Mar 8, 2025

-n 1 allows to have one empty line.

This looks better to me than the first idea.

So there is no way to disallow empty lines all-together.

Why not 0 for this and -1 for unlimited?

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

I think this is a useful addition.
Not allowing any empty line is maybe not a feature sought after in the context of fix-whitespace, so I think having 0 switching off the check (and 1 for normalizing subsequent empty lines to a single one) is ok.

So, happy to merge if that one comment is addressed.

@phadej
Copy link
Author

phadej commented Mar 8, 2025

Please suggest concrete changes, my English is quite bad (I feel i'm very often misunderstood), so instead of going back and forth on comments and wording, just propose what to write.

If you cannot, I cannot either.

@phadej
Copy link
Author

phadej commented Mar 14, 2025

Please pick all the colors for the bikeshed at once if you can; or I'll just ask ChatGPT to make the changes for you.

@ulysses4ever
Copy link
Contributor

I don't want to derail this already perfectly useful patch, so I'll try to stay away. Also just FYI I don't have the approve bits.

Co-authored-by: Artem Pelenitsyn <[email protected]>
@phadej
Copy link
Author

phadej commented Mar 30, 2025

ping. I think I addressed the comment already. Please merge.

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.

An option to squash consequtive newlines.
3 participants