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

Ignore line ending characters when matching code issues against baseline #5979

Closed
bdsl opened this issue Jun 22, 2021 · 13 comments · Fixed by #6000
Closed

Ignore line ending characters when matching code issues against baseline #5979

bdsl opened this issue Jun 22, 2021 · 13 comments · Fixed by #6000

Comments

@bdsl
Copy link
Contributor

bdsl commented Jun 22, 2021

I was puzzling over why my Psalm run was passing locally, but had many errors in CI. The answer was that I had autocrlf turned on in git settings, so my local PHP files were in a different format to those generated in the CI environment. Specifically I had many <code> elements in the baseline that ended with &#13;</code>

I wonder if it would be worth making Psalm ignore the &#13; (carriage return), and also not add it when generating the baseline.

@psalm-github-bot
Copy link

Hey @bdsl, can you reproduce the issue on https://psalm.dev ?

@bdsl
Copy link
Contributor Author

bdsl commented Jun 22, 2021

@psalm-github-bot sorry, psalm.dev doesn't support baselines.

@weirdan
Copy link
Collaborator

weirdan commented Jun 23, 2021

Remind me how that works: you have CR LF line endings locally, but CR is stripped during commit and reinstated during checkout, right? So CR is never added to the repository and thus is missing on machines with autocrlf disabled?

@bdsl
Copy link
Contributor Author

bdsl commented Jun 23, 2021

In my case I had LF endings locally on a linux machine and CR LF endings on the PHP files in the repo. Git with autocrlf=true converts the php files to my local format on checkout.

Then when I generate a baseline file locally it doesn't match the files as they are in the repository, so Psalm reports errors in the CI environment where autocrlf is set to false.

@weirdan
Copy link
Collaborator

weirdan commented Jun 23, 2021

But if you don't have CRs locally how they ended up in the baseline (&#13; is encoded CR)?

@weirdan
Copy link
Collaborator

weirdan commented Jun 23, 2021

Also, docs say autocrlf always causes files to be stored in the repo with LF line endings.

@bdsl
Copy link
Contributor Author

bdsl commented Jun 23, 2021

Hmm, I'm not 100% sure now - looks like at list some of the files must have had CRLF on my local - quite possibly there was a mixture of both formats, maybe between files maybe even within one file. I'm not entirely sure - I just know that I fixed the issue by generating a new baseline within the CI executor and committing that, and the diff on that commit showed the removal of &#13; entities.

@reeperbahnause
Copy link

Hey,
I'm facing the same behavior.
In our case the line ending in the git repo is LF. The psalm baseline file was created on a linux dev environment.
Once I checkout the branch for testing psalm on windows the line endings will be changed to CRLF because of the git client configuration autocrlf
Running psalm on windows won't match the baseline rules because of the extra character (for example at the end of a function definition)

Here is an example of the linux baseline file vs the windows baseline file:

image

@bdsl
Copy link
Contributor Author

bdsl commented Jun 25, 2021

@weirdan I'm happy to try and PR an implementation for this if you think it's the right thing to do for Psalm.

@weirdan
Copy link
Collaborator

weirdan commented Jun 25, 2021

@bdsl sure, please do

@bdsl
Copy link
Contributor Author

bdsl commented Jun 25, 2021

Will try and get that in in the next few days.

bdsl added a commit to bdsl/psalm that referenced this issue Jun 26, 2021
bdsl added a commit to bdsl/psalm that referenced this issue Jun 26, 2021
…t baseline

Should fix issue vimeo#5979

We could also trim code issues when writing to baseline, but I think
that's a minor BC break, so probably shouldn't happen until Psalm 9.
bdsl added a commit to bdsl/psalm that referenced this issue Jun 26, 2021
…t baseline

Should fix issue vimeo#5979

We could also trim code issues when writing to baseline, but I think
that's a minor BC break, so probably shouldn't happen until Psalm 9.
@bdsl
Copy link
Contributor Author

bdsl commented Jun 27, 2021

@weirdan btw, is there a set policy on when to close Psalm issues? Might it make sense to leave it open until there's a release available with the fix?

@weirdan
Copy link
Collaborator

weirdan commented Jun 27, 2021

That would require some additional automation, while closing them when PR is merged is a Github's native functionality.

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 a pull request may close this issue.

3 participants