Ignore line endings in exclude-file

[Jackenmen]

Fixes #1888

[peternewman]

Can you extend test_ignore_dictionary too, so that also covers both cases?

[Jackenmen]

Alright, I added the test cases for all combinations of these line endings \n, \r\n, \n, \r\n.

It seems that I need to use different misspellings for test_ignore_dictionary as the ones I chose don't seem to be detected as misspellings and therefore the test cases don't quite work. Got any suggestions on what words I could use here?

[Jackenmen]

Got any suggestions on what words I could use here?

Nevermind, I found some in the other tests 😄

[peternewman]

Sorry the write changes thing will need fixing somehow. Perhaps just move it to when you're comparing the line to the exclude list?

[peternewman]

I've realised the issue. This will write back out. See the -w based tests (and extend them to prove it/stop us breaking it anyway).

An input of abandonned\nAbandonned\r\nABANDONNED \nAbAnDoNnEd will become abandonned\nAbandonned\nABANDONNED\nAbAnDoNnEd I think

[Jackenmen]

An input of abandonned\nAbandonned\r\nABANDONNED \nAbAnDoNnEd will become abandonned\nAbandonned\nABANDONNED\nAbAnDoNnEd I think

It becomes abandoned\nAbandoned\nABANDONED \nabandoned on both codespell 2.0.0 and on the version with modifications from this PR.
I'm not quite sure what and why I should be extending tests here as the changes from this PR do not make any changes that would cause any regression. I feel like it would be better suited for a separate PR?

If you don't agree, I can make the changes here, just let me know what test I should be modifying and/or what kind of inputs/outputs should I be checking there.

[bl-ue]

ping @peternewman

[Jackenmen]

@peternewman do you think I could get your decision here so that I know if this is fine or if I should work on something before this will be able to move forward?

[tenshiAMD]

ping @peternewman

[peternewman]

An input of abandonned\nAbandonned\r\nABANDONNED \nAbAnDoNnEd will become abandonned\nAbandonned\nABANDONNED\nAbAnDoNnEd I think

It becomes abandoned\nAbandoned\nABANDONED \nabandoned on both codespell 2.0.0 and on the version with modifications from this PR.

Odd, as we added the tests here in #2490 and they seem to behave okay, although admittedly they are a slightly different code path to the command line version. What OS were you testing on?

I'm not quite sure what and why I should be extending tests here as the changes from this PR do not make any changes that would cause any regression. I feel like it would be better suited for a separate PR?

I believe the change you're making will introduce a bug, or perhaps simply further embed an existing bug, so I figure fixing the crucial underlying one first is probably a good idea.

If you don't agree, I can make the changes here, just let me know what test I should be modifying and/or what kind of inputs/outputs should I be checking there.

As mentioned, I think we've now done that in #2490 so when that's in, you can just update/rebase and as long as your code continue to pass we'll be all good.

[Jackenmen]

What OS were you testing on?

Back then probably only on Windows 10.

I believe the change you're making will introduce a bug, or perhaps simply further embed an existing bug, so I figure fixing the crucial underlying one first is probably a good idea.

That certainly makes sense, I am just not really sure what the issue is exactly. Hopefully, the tests added in #2490 will make it more clear whether there is any issue and if so, what that issue is, thanks for working on those tests. Once that PR gets merged, I'll rebase.
If any further work on the PR will be needed, I might not get to it before Saturday but other than that, I'll be happy to make any necessary changes 👍

[Jackenmen]

I rebased the PR on top of current master now that the tests got merged. Locally the tests ran fine, you'll have to approve the CI run though since I haven't had any PRs into this repository yet.

---

By the way, in case you don't know, the requirement for approval of workflows for first-time contributors who are NOT new to GitHub can be disabled to decrease the contribution barrier for first-time contributors, see:
https://twitter.com/JakubKuczys/status/1562660410108841986

[luzpaz]

@peternewman care to review this?

[Jackenmen]

I resolved conflicts in case anyone wants to look at this again.

[DimitriPapadopoulos]

BTW, it would be great to make sure all options that take a file as an argument are immune to extra line endings in the file:

• -D DICTIONARY
• -I FILE
• -x FILE
• --config CONFIG
• --toml TOML

But that would be a different PR, once this one is merged 😄

[Jackenmen]

If this one ever gets merged, I may look into it.

[DimitriPapadopoulos]

@peternewman Would you agree to merge this?

[Andy-Grigg]

Is there any interest in getting this merged? As far as I can tell, the current implementation cannot work at all with Windows line endings in the exclude-lines file

open() is called in build_exclude_hashes without the newline parameter, which normalizes all line endings to \n, but everywhere else open() is called with newline='', so newlines are preserved.

As a result, if there's a \r\n ending in the file to be checked, it's impossible to match any line to the exclude-lines file.

[Jackenmen]

This PR is supposed to cause line endings in spell-checked files to be ignored when being compared to entries in exclude lines file. This feature is wanted and I definitely have interest in getting this PR merged. You are the first to say that the current implementation has issues (more specifically, you claim that it doesn't work) so there was no way for me to know there are some concerns about it before you posted your comment :p

I believe the implementation is sound though - the lines are rstripped before checking so the way files are opened shouldn't matter. Test cases also seem to indicate that, are you able to provide reproduction steps for which the changes in this PR aren't working as intended?

[Andy-Grigg]

This PR is supposed to cause line endings in spell-checked files to be ignored when being compared to entries in exclude lines file. This feature is wanted and I definitely have interest in getting this PR merged. You are the first to say that the current implementation has issues (more specifically, you claim that it doesn't work) so there was no way for me to know there are some concerns about it before you posted your comment :p

I believe the implementation is sound though - the lines are rstripped before checking so the way files are opened shouldn't matter. Test cases also seem to indicate that, are you able to provide reproduction steps for which the changes in this PR aren't working as intended?

I'm sorry, I wasn't clear in my comment. This work is brilliant and I love it, I just checked out your branch and the ignore-lines functionality works as expected. As you say, since you're stripping off the whitespace, it doesn't matter how open() is set up.

'Current implementation' in my comment was referring to the released functionality, not the changes that you've done here! My comment was addressed more to the maintainer - I was really just trying to add my vote for getting this merged.

[DimitriPapadopoulos]

@Jackenmen Could you rebase one last time, as there are conflicts? I'll merge when you're done.

All raised issues seem to have been addressed, including with extensive tests.

[Jackenmen]

Could you rebase one last time, as there are conflicts?

@DimitriPapadopoulos It doesn't seem that there were any conflicts, I just made a regular merge commit through GitHub UI. Do let me know if you're not using squash merges and need me to rebase instead to keep a clean history, I can do that instead 😄

[Jackenmen]

'Current implementation' in my comment was referring to the released functionality, not the changes that you've done here!

Makes sense, both our interpretations of your comment were definitely valid, just the wonders of misunderstandings in text-based communication :) I guess I just assumed it's about my PR since that's where your comment was posted + you also went into details of the implementation (but as it turns out, they were details of the implementation in master as well). No worries though, I'm just glad you like it!

[DimitriPapadopoulos]

@Jackenmen Yes, I was thinking of squash merging.

[Jackenmen]

No need for me to do anything else then, great! Excited to see this finally get into the project!

[DimitriPapadopoulos]

@Jackenmen Thank you for your contribution and your patience.