search

moodlehq / moodle-cs

Moodle Coding Style
https://github.com/moodlehq/moodle-cs
GNU General Public License v3.0
18 stars 15 forks source link

Fixes #176. Match blank boilerplate lines correctly. #177

Closed micaherne closed 2 months ago

micaherne commented 2 months ago

This patch fixes an issue that contributes to #176 where lines of the boilerplate that should be blank are not identified as wrong if they have text in them.

This is because we are only checking that the lines start with the expected text. I'm not sure why this is but it was this way in the original code before I added the fixes.

I think this fixes #176 in a way, although there is still a weird chain of fixing going on when there's a CRLF in the opening PHP tag. I'll add some more to that issue about it just in case it comes up again.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.11%. Comparing base (f7668d9) to head (e6d2726).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #177 +/- ## ========================================= Coverage 98.11% 98.11% - Complexity 951 952 +1 ========================================= Files 40 40 Lines 2816 2818 +2 ========================================= + Hits 2763 2765 +2 Misses 53 53 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

stronk7 commented 2 months ago

Hi @micaherne , regarding CRLF ended lines... I think we shouldn't apply for any specific workaround or fix ever.

Moodle files are supposed to be, always, LF ended (Unix ended) and I think that the correct and safe thing to do is to, always, assume that's the case.

If there is any file having other line endings... then for sure some other sniff (Generic.Files.LineEndings is part of our standard) should detect that and report it, but we cannot make all the code conditional on those wrong line endings.

So it's good that we aren't adding any specific CRLF code here (or elsewhere, ever). IMO. Because that code is not allowed. As simple as that.

Ciao :-)

micaherne commented 2 months ago

Thanks @stronk7! I totally agree about CRLF endings - I don't think it makes any sense to add extra code to work around them - I was just a bit confused about how they were affecting the other fixes. But anyway it seems to be giving the expected output with them anyway so I'm happy with that 😄