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

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

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

micaherne
Copy link
Contributor

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.

Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@micaherne micaherne force-pushed the 176-fix-boilerplate-update branch from 2b829d7 to e6d2726 Compare July 4, 2024 14:55
@stronk7
Copy link
Member

stronk7 commented Jul 4, 2024

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

Copy link
Member

@stronk7 stronk7 left a comment

Choose a reason for hiding this comment

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

Other than the comment above, that is just a reflexion... the changes look spot-on and are perfectly covered, so I think this is ready to be merged.

Thanks!

@stronk7 stronk7 merged commit 5f0d1f2 into moodlehq:main Jul 4, 2024
13 checks passed
@micaherne micaherne deleted the 176-fix-boilerplate-update branch July 4, 2024 16:14
@micaherne
Copy link
Contributor Author

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 😄

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.

Inserted boilerplate gets broken if there is a CRLF line break after the opening tag
2 participants