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.

Sign up for GitHub

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

Jump to bottom

The new version v2.2.4 generates false positive output #2786

Closed
pirat89 opened this issue Mar 10, 2023 · 7 comments
Closed

The new version v2.2.4 generates false positive output #2786

pirat89 opened this issue Mar 10, 2023 · 7 comments
Labels
question

Comments

@pirat89
Copy link

pirat89 commented Mar 10, 2023

The new codespell version released yesterday started to generate false positives when

  • a string is composed dynamically

  • when string contains escaped characters , so e.g. following strings are reported as mispelled:

  • "My repositor{suffix} ...".format(suffix=suffix)

  • 'We couldn\'t do ....'

Additional info:

Error: ./repos/system_upgrade/common/actors/localreposinhibit/actor.py:60: repositor ==> repository
Error: ./repos/system_upgrade/common/actors/localreposinhibit/actor.py:67: repositor ==> repository
Error: ./repos/system_upgrade/common/actors/checktargetiso/libraries/check_target_iso.py:149: repositor ==> repository
Error: ./repos/system_upgrade/common/actors/selinux/selinuxapplycustom/actor.py:154: couldn ==> could, couldn't
Error: ./repos/system_upgrade/common/actors/selinux/selinuxapplycustom/actor.py:162: couldn ==> could, couldn't
@pirat89 pirat89 mentioned this issue Mar 10, 2023
Merged
@DimitriPapadopoulos
Copy link
Collaborator

That has always been the case. Are you certain this is a regression?

@pirat89
Copy link
Author

pirat89 commented Mar 10, 2023 •
edited
Loading

@DimitriPapadopoulos the mentioned code has been part of the upstream for a longer time and tests (including codespell) have been passing. e.g. like in this one PR oamg/leapp-repository#979 that introduced the chacktargetiso actor. It's visible that it passed originally and the same code is failing the test now. Also the previous run in the PR has been ok and it started to faile just today.

It's possible I missed something, but all clues I have seem to be suggesting it's regression.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Mar 10, 2023 •
edited
Loading

OK, let me see:

$ cat > foo.bar.txt
My repositor{suffix}
We couldn\'t do
$

With codespell 2.2.2:

$ codespell foo.bar.txt
$

With codespell 2.2.3:

$ codespell foo.bar.txt
foo.bar.txt:1: repositor ==> repository
foo.bar.txt:2: couldn ==> could, couldn't
$

It's just that repositor and couldn have been added to the dictionary of typos:

@DimitriPapadopoulos
Copy link
Collaborator

This does raise the question of dictionary updates, especially in CI actions.

@pirat89
Copy link
Author

pirat89 commented Mar 10, 2023 •
edited
Loading

@DimitriPapadopoulos thanks for the info. It make sense. So I will add it to the ignorelist in our projects.

pirat89 added a commit to pirat89/leapp-repository that referenced this issue Mar 10, 2023
@pirat89
Update codespell ignorelist: couldn,repositor
4a14f74 
The new version of codespell contains additional "typos" for the
detection in the dictionary, which produces FP fails in tests
as typos are detected also in cases like:
    couldn\'t
    repositor{suffix}
etc. For now, we will just update the ignorelist, but in future
it would be ideal to not generate such cases. Doing differences
between singular/plural is not providing big benefit in report.
Escaping is not so problematic I would say, but in case of issues,
we could just switch to longer form - like "could not".
But there is no beenfit to update the existing code now, so let's
focus in future on better texts and keep the existing strings as
they are until they are reworded due to additional wanted changes
(I mean, if there is any additional reason in future to change them).

FYI:
  codespell-project/codespell#2786
@pirat89 pirat89 mentioned this issue Mar 10, 2023
Merged
pirat89 added a commit to oamg/leapp-repository that referenced this issue Mar 10, 2023
@pirat89
Update codespell ignorelist: couldn,repositor
6aee6d7 
The new version of codespell contains additional "typos" for the
detection in the dictionary, which produces FP fails in tests
as typos are detected also in cases like:
    couldn\'t
    repositor{suffix}
etc. For now, we will just update the ignorelist, but in future
it would be ideal to not generate such cases. Doing differences
between singular/plural is not providing big benefit in report.
Escaping is not so problematic I would say, but in case of issues,
we could just switch to longer form - like "could not".
But there is no beenfit to update the existing code now, so let's
focus in future on better texts and keep the existing strings as
they are until they are reworded due to additional wanted changes
(I mean, if there is any additional reason in future to change them).

FYI:
  codespell-project/codespell#2786
@DimitriPapadopoulos DimitriPapadopoulos mentioned this issue Mar 12, 2023
Merged
@bdice

This comment was marked as off-topic.

Sign in to view
@DimitriPapadopoulos

This comment was marked as off-topic.

Sign in to view
@bdice bdice mentioned this issue Dec 17, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@pirat89 @DimitriPapadopoulos @bdice