Behavior of --skip with files list

[rkm]

This was partially discussed in #1498.

Attempting to --skip a single file which is also present in the file list or is matched by the glob pattern causes the file to be checked anyway:

$ ls
bar.md  foo.md

$ codespell --skip foo.md *.md
bar.md:1: abandonned ==> abandoned
foo.md:1: abandonned ==> abandoned

$ codespell --skip foo.md -- foo.md bar.md ...
bar.md:1: abandonned ==> abandoned
foo.md:1: abandonned ==> abandoned

Is this the intended behavior or a bug? If this is intentional, can it be made more obvious in the documentation? Thanks!

[peternewman]

It seems it's just because we only check the skip if it's a directory. Should be a fairly easy fix.

[rkm]

I might be able to take a look at this if you'd like?

[peternewman]

Feel free if you want to @rkm . Some TDD would be good, i.e. add a test that breaks it and confirm the CI catches it, then apply the fix.

I think globs can muddy the water a bit as the shell may well expand them before passing them to codespell. Although I must admit when I did a few tests of skip adding it into our action it seemed to behave in some rather odd ways to me.

[peternewman]

Also these should all work the same:

• --skip ./foo/ - Currently doesn't work
• --skip ./foo - Currently works
• --skip foo/ - Untested
• --skip foo - Untested

We also need to check if --skip ./foo is equivalent to --skip ./foobar (on a file/folder named foobar) and likewise for --skip foo.

[rkm]

hi, sorry for the inactivity on this. I started to add some tests as you suggested, but not had much time recently. Looking at the actual source, I'm also not clear on how to best implement the changes. I think we'll have to modify the GlobMatch class to start with at least. Suggestions welcome!

[peternewman]

hi, sorry for the inactivity on this.

No worries @rkm .

I started to add some tests as you suggested, but not had much time recently. Looking at the actual source, I'm also not clear on how to best implement the changes. I think we'll have to modify the GlobMatch class to start with at least. Suggestions welcome!

I think a chunk of the basic stuff will be fixed by just adding a:

if glob_match.match(root):
    continue

Before the is_hidden test within for filename in options.files: although you'll probably need to see some of the other tests to get the shell to do glob expansion to allow your test to work as it would in real life. Certainly that should fix your simple example.

For the other stuff its probably a good start to write some simple standalone tests of our GlobMatch class.

There's also this module, but it treats hidden files differently which might not be exactly the behaviour we want:
https://docs.python.org/3/library/glob.html#module-glob

[peternewman]

Hi @rkm ,

Have you had any more time to look at this? What you've got in master...rkm:issue/1528-skip-files looks promising!

[rkm]

Apologies, going to have to un-assign myself from this :(

[peternewman]

No worries @rkm , thanks for what you've done, we should be able to build on those test and get somewhere hopefully.

[bwitt]

Hi I have a fix in
#1913
but can't get the coverage up for some reason?

[peternewman]

Thanks for fixing the first two @bwitt:

codespell --skip foo.md *.md
codespell --skip foo.md -- foo.md bar.md

As far as I'm aware you've not touched these ones (or not added tests for them) at least:

Also these should all work the same:

• --skip ./foo/ - Currently doesn't work
• --skip ./foo - Currently works
• --skip foo/ - Untested
• --skip foo - Untested

We also need to check if --skip ./foo is equivalent to --skip ./foobar (on a file/folder named foobar) and likewise for --skip foo.

So I've created #1915 to track that. Do you fancy adding tests for them (and fixing if required)?