Add support for ignoring spelling mistakes in URIs specifically. by jonmeow · Pull Request #1656 · codespell-project/codespell

jonmeow · 2020-08-28T21:27:46Z

This is per discussion on #676, particularly:
#676 (comment)

The URI/email regex is deliberately simple to help with maintainability. False positives are possible but would likely have a low cost associated, particularly as nothing is ignored without setting --uri-ignore-words-list. False negatives would probably cause more trouble, so the regex is over-inclusive.

peternewman · 2020-09-04T15:32:13Z

I suspect we should potentially leave discussion of the regex itself to a separate PR to avoid potentially derailing this one, but here is something interesting to look at/test:
https://mathiasbynens.be/demo/url-regex

peternewman

A few comments.

peternewman · 2020-09-04T15:38:21Z

+ for uri_word in extract_words(uri, word_regex,
+ ignore_word_regex):
+ if "*" in uri_ignore_words or uri_word in uri_ignore_words:
+ check_words.remove(uri_word)


Can you not just drop the entire uri if * is present? Like your previous PR.
Also won't this currently mean that:
I like to eat foo you can buy it from my online store at http://example.com/foo
Would miss foo->food in the free text because it exists in the URI if I set --uri-ignore-word-list=foo?

Edit, I see your test says not. Is it actually finding the right one, or just working because there are two hits on the line and it just removes one of them?

It's working because there are two hits on the line, and removing one. codespell doesn't track where the hit was when doing spell corrections, so if there is a repeated misspelling on the line, and it's told to fix the spelling, it'll probably fix both. I don't think that's particularly bad though: while something might be ignored in a uri, if it's there in plain text, I'd expect the common case to be that it needs to either be fixed in both or ignored in both. (either way, this is an edge case scenario)
If you prefer though, I could switch things around to do something more like:
if "*" in uri_ignore_words: line = uri_regex.sub('', line) check_words = extract_words(line, word, ignore_word_regex) if "*" not in uri_ignore_words: apply_uri_ignore_words(...)
I hadn't gone that route just because it felt more symmetric to use a similar approach with "*"

I wonder if something even more like this might make more sense. Especially because your initial comment was people will just want to skip all typos in URIs wasn't it? Which means that 90% of the time we just want to hide the URI rather than process it twice.
Suggested change
foruri_wordinextract_words(uri, word_regex,
ignore_word_regex):
if"*"inuri_ignore_wordsoruri_wordinuri_ignore_words:
check_words.remove(uri_word)
if"*"inuri_ignore_words:
text=uri_regex.sub(' ', text)
returnword_regex.findall(text)
defapply_uri_ignore_words(check_words, line, word_regex, ignore_word_regex,
uri_regex, uri_ignore_words):
foruriinre.findall(uri_regex, line):
foruri_wordinextract_words(uri, word_regex,
ignore_word_regex):
ifuri_wordinuri_ignore_words:
check_words.remove(uri_word)
@larsoner do you have any strong feelings either way?

No strong opinion, either way seems fine to me

I've switched the code to check for "*". I don't think it's a performance issue, but whatever's easiest to understand.
I'm not clear what the suggested edit was intended for. If the top half was supposed to be rewriting extract_words, I'd note that extract_words is also used for the options.check_filenames code path, where I felt URI checks shouldn't be done (felt more likely to hit false positives).

jonmeow · 2020-09-08T17:43:46Z

I suspect we should potentially leave discussion of the regex itself to a separate PR to avoid potentially derailing this one, but here is something interesting to look at/test:
https://mathiasbynens.be/demo/url-regex

Added tests. I've loosened the regex a little accordingly, too.

peternewman

A few more comments, I still need to take a final look at the main code changes.

jonmeow

Added a few more tests based on comments.

jonmeow · 2021-04-20T16:06:19Z

Just checking here, is there still a chance of getting this merged? If not, I'll probably close the PR given its age.

larsoner · 2021-04-21T18:23:49Z

@peternewman did you want to look again?

I'm going to close and reopen so that CIs get restarted -- there are some marked as pending which is weird. But I don't see a problem with merging this given the positive reception from @peternewman before, so feel free to ping me in a few days if you haven't heard anything @jonmeow !

peternewman · 2021-04-23T09:46:36Z

Sorry @jonmeow . I suck, I'll make sure to have a look at this again later today.

peternewman · 2021-04-23T14:54:19Z

+ '# Please see mailto:abandonned@example.com?subject=Test'
+ ' for info\n'):


I don't know if this is actually a match or a miss with the regex, but would be good to get "documented" in a test:
Suggested change
'# Please see mailto:abandonned@example.com?subject=Test'
' for info\n'):
'# Please see mailto:abandonned@example.com?subject=Test'
' for info\n',
'# Please see mailto:foo@example.com?subject=Test'
'abandonned\n',):

@jonmeow this one.

This suggestion is missing a space in Testabandonned, but I'm assuming that's an oversight rather than deliberate: added a test around line 633 similar to the test on line 581.

Oops, it was indeed @jonmeow . I was actually thinking of the example where the subject of the email was "Test abandonned", which I guess probably needs to be entered as "Test%20abandonned" or similar, which probably breaks any cleverness we have in place currently.

peternewman

Just a few minor comments if you wouldn't mind addressing them.

Really sorry again for the delay.

jonmeow · 2021-05-27T16:11:25Z

Just pinging again, I don't think I can merge this because I don't have write access

peternewman · 2021-05-28T02:15:49Z

Just pinging again, I don't think I can merge this because I don't have write access

Hi @jonmeow,

I left a few comments in a hopefully final review after you pinged last time, although only one ended up attached to the review, the other was just a comment. Would you mind addressing them?

@bl-ue 's comments don't seem unreasonable (and pretty quick fixes) if you're happy to look after them.

Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>

…to ignore-uris

jonmeow · 2021-06-01T18:12:58Z

Just pinging again, I don't think I can merge this because I don't have write access
Hi @jonmeow,
I left a few comments in a hopefully final review after you pinged last time, although only one ended up attached to the review, the other was just a comment. Would you mind addressing them?
@bl-ue 's comments don't seem unreasonable (and pretty quick fixes) if you're happy to look after them.

Done.

bl-ue

LGTM.

@jonmeow did you see https://github.com/codespell-project/codespell/pull/1656/files#r619284923?

jonmeow

LGTM.
@jonmeow did you see https://github.com/codespell-project/codespell/pull/1656/files#r619284923?

Thanks, I wasn't seeing that.

jonmeow · 2021-06-01T22:19:59Z

+ '# Please see mailto:abandonned@example.com?subject=Test'
+ ' for info\n'):


This suggestion is missing a space in Testabandonned, but I'm assuming that's an oversight rather than deliberate: added a test around line 633 similar to the test on line 581.

peternewman

Thanks very much for this @jonmeow it looks great. Sorry it's taken quite so long to get merged!

jonmeow added 3 commits August 28, 2020 14:23

Add support for ignoring spelling mistakes in URIs specifically.
1e7f5f7

Fix flake8
b8db3f0

Remove duplicate test
8069358

peternewman requested changes Sep 4, 2020
View reviewed changes

Tests and option
f315b0d

jonmeow requested a review from peternewman October 7, 2020 23:34

peternewman requested changes Oct 10, 2020
View reviewed changes

Comment threadcodespell_lib/tests/test_basic.py
Comment threadcodespell_lib/tests/test_basic.py
Comment threadcodespell_lib/_codespell.py
Comment threadcodespell_lib/_codespell.py
Comment threadcodespell_lib/_codespell.py
Comment threadcodespell_lib/_codespell.py

jonmeow added 2 commits October 12, 2020 14:15

More tests
909f0dc

Rephrase rejected to un-ignored
6bb3ef4

jonmeow commented Oct 12, 2020
View reviewed changes

Comment threadcodespell_lib/tests/test_basic.py
Comment threadcodespell_lib/tests/test_basic.py
Comment threadcodespell_lib/_codespell.py
Comment threadcodespell_lib/_codespell.py
Comment threadcodespell_lib/_codespell.py
Comment threadcodespell_lib/_codespell.py

jonmeow requested a review from peternewman October 12, 2020 21:17

jonmeow added 2 commits November 6, 2020 11:54

Restructure * handling
b5b82e8

Merge remote-tracking branch 'upstream/master' into ignore-uris
a5402ec

larsoner closed this Apr 21, 2021

larsoner reopened this Apr 21, 2021

peternewman mentioned this pull request Apr 23, 2021
Interactive Mode and Multiple Typos of the same type on a line #1910
Open

peternewman reviewed Apr 23, 2021
View reviewed changes

peternewman approved these changes Apr 23, 2021
View reviewed changes

Comment threadcodespell_lib/_codespell.py Outdated

bl-ue reviewed May 27, 2021
View reviewed changes

Comment threadcodespell_lib/_codespell.py Outdated
Comment threadcodespell_lib/_codespell.py Outdated

peternewman added the enhancement label May 28, 2021

peternewman added this to the v2.1.0 milestone May 28, 2021

jonmeowand others added 2 commits June 1, 2021 10:17

Update codespell_lib/_codespell.py
f7e3e73
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>

regex
4961cc1

Merge branch 'ignore-uris' ofhttps://github.com/jonmeow/codespellin…
4f7d81b
…to ignore-uris

flake8
6019d3d

bl-ue approved these changes Jun 1, 2021
View reviewed changes

jonmeow commented Jun 1, 2021
View reviewed changes

test
ad72118

peternewman approved these changes Jun 1, 2021
View reviewed changes

peternewman merged commit 0338e3c into codespell-project:masterJun 1, 2021

peternewman linked an issue Jun 1, 2021 that may be closed by this pull request
Don't report spelling mistakes in HTTP URLs (e.g. in comments) #676
Closed

peternewman mentioned this pull request Jun 1, 2021
Don't report spelling mistakes in HTTP URLs (e.g. in comments) #676
Closed

jonmeow deleted the ignore-uris branch April 23, 2024 17:01

-foruri_wordinextract_words(uri, word_regex,
-ignore_word_regex):
-if"*"inuri_ignore_wordsoruri_wordinuri_ignore_words:
-check_words.remove(uri_word)
+if"*"inuri_ignore_words:
+text=uri_regex.sub(' ', text)
+returnword_regex.findall(text)
+defapply_uri_ignore_words(check_words, line, word_regex, ignore_word_regex,
+uri_regex, uri_ignore_words):
+foruriinre.findall(uri_regex, line):
+foruri_wordinextract_words(uri, word_regex,
+ignore_word_regex):
+ifuri_wordinuri_ignore_words:
+check_words.remove(uri_word)

		'# Please see mailto:abandonned@example.com?subject=Test'
		' for info\n'):

Uh oh!

Conversation

jonmeow commented Aug 28, 2020

Uh oh!

peternewman commented Sep 4, 2020

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonmeowSep 8, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonmeow commented Sep 8, 2020

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonmeow commented Apr 20, 2021

Uh oh!

larsoner commented Apr 21, 2021

Uh oh!

peternewman commented Apr 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peternewman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonmeow commented May 27, 2021

Uh oh!

Uh oh!

Uh oh!

peternewman commented May 28, 2021

Uh oh!

jonmeow commented Jun 1, 2021

Uh oh!

bl-ue left a comment

Choose a reason for hiding this comment

Uh oh!

jonmeow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peternewman left a comment

jonmeowSep 8, 2020•
edited
Loading