search

adrienverge / yamllint

A linter for YAML files.
GNU General Public License v3.0
2.9k stars 278 forks source link

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

Open dbk-rabel opened 1 month ago

dbk-rabel commented 1 month ago

fixes #475

coveralls commented 1 month ago

Coverage Status

coverage: 99.825%. remained the same when pulling 75150b329e6ac0bf382a72771a05808dfe2f63b4 on dbk-rabel:patch-1 into 95e17b33dc147c9c35742853e17bdf3fd508550b on adrienverge:master.

dbk-rabel commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Hello David,

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

I'm :+1: 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 commented 1 month ago

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 commented 3 weeks ago

@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 commented 2 weeks ago

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

dbk-rabel commented 1 week ago

Salut Adrien.

My idea with the new parameter disable_after_first_occurence was to be able not to hardcode some special behaviour for one specific rule in the linter.py . So I implemented some general parameter that than was used only by one of the rules.

My latest push would be the alternative: Tell the linter explicitly to handle "new-lines" differently than all other rules.

I fear that you might not like this solution either. But I'm giving my best and this is the cleanest solution I came up with.

Yours David