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

Apply new-lines check to all lines of a file, not just the first one #693

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

Conversation

dbk-rabel
Copy link

fixes #475

@coveralls
Copy link

coveralls commented Oct 17, 2024

Coverage Status

coverage: 99.825%. remained the same
when pulling 55de11c on dbk-rabel:patch-1
into 95e17b3 on adrienverge:master.

Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

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

Hello David,

Thanks for contributing!

Could you update the commit to report only the first wrong line (at most once per file)? Cf. discussion on the issue tracker #475, especially #475 (comment)

@dbk-rabel
Copy link
Author

Hello David,

Thanks for contributing!

Could you update the commit to report only the first wrong line (at most once per file)? Cf. discussion on the issue tracker #475, especially #475 (comment)

Hello Adrian.
I think I missunderstood in the first place. Sorry.
Yes, I can update the commit. I'm just not a 100% sure how this is done the cleanest way. Are there other checks that implement the same thing? (I.e. checking the whole file but only yielding the first occurence?)

@dbk-rabel dbk-rabel force-pushed the patch-1 branch 2 times, most recently from 0d0fd7d to b6c01d2 Compare October 21, 2024 09:03
@dbk-rabel
Copy link
Author

Sorry, I don't seem to find a way to do it. I think we would somehow have to keep track of the files we already found an erronous line in. I'm not quite sure how this should be done and I doubt it can be done elegantly.

To be honest, I do not agree with the discussion in #475 . I personally think that it would be correct to produce a warning for each line.

I'd still love to implement it, if there was a good way to do it. Maybe you could help @adrienverge ?

@adrienverge
Copy link
Owner

Hello David,

I personally think that it would be correct to produce a warning for each line.

I'm 👍 to fix a bug (wrong newlines are currently not detected if on line 2+) but it would be a bad idea to change current behavior, i.e. report 1000 errors for files containing 1000 lines, where 1 were previously reported. Many users would complain, and in my opinion they would be right. And it would be inconsistent with rule syntax.

Maybe you could help @adrienverge ?

I'm willing to review and release ready-to-merge PRs on my free time, but at the moment I don't have the bandwidth to implement nor guide implementation of development requests, sorry :/

@dbk-rabel
Copy link
Author

I'm 👍 to fix a bug (wrong newlines are currently not detected if on line 2+) but it would be a bad idea to change current behavior, i.e. report 1000 errors for files containing 1000 lines, where 1 were previously reported. Many users would complain, and in my opinion they would be right. And it would be inconsistent with rule syntax.

You convinced me. I agree: People would be upset and would complain. So such a drastic change in behaviour is probably a bad idea.

It still leaves me a litte bit sad, because I do think that 1000 errors would be the correct behaviour. :) But the argument holds: To change behaviour so drastically over night is probably never a good idea.

Maybe you could help @adrienverge ?

I'm willing to review and release ready-to-merge PRs on my free time, but at the moment I don't have the bandwidth to implement nor guide implementation of development requests, sorry :/

No worries. I actually wasn't aware that yamllint is something you maintain in your free time. My misstake. When a tool is used so widely, I always assume that there is some paid developers behind it.

Sorry if I was too demanding. I really appreciate your work, Thank you!

I hope I will find the time to take a closer look at this issue. But I'm not sure yet when that will be the case.

@dbk-rabel
Copy link
Author

@adrienverge What is your opinion on my today's approach? Would something like this be an alternative?

It feels a little bit unclean. But it seems to work, does not break current behaviour and could be used by other rules as well in the future, if needed.

@adrienverge
Copy link
Owner

Hello David,

It looks like your last push added a new rule option disable_after_first_occurence, which is not really what we want.

The goal is rather to fix the existing rule, by reporting the first wrong newline character even if it's not on the first line (but only the wrong newline: at most once per file).

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.

rule new-lines: checks only the first newline character in a file
3 participants