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

Detection of string delimiters #3305

Open
mdeweerd opened this issue Jan 23, 2024 · 15 comments · May be fixed by #3588 or #3596
Open

Detection of string delimiters #3305

mdeweerd opened this issue Jan 23, 2024 · 15 comments · May be fixed by #3588 or #3596
Labels

Comments

@mdeweerd
Copy link
Contributor

mdeweerd commented Jan 23, 2024

Currently, words in double quoted strings are spellchecked, but not strings with simple quotes.

My suggestion is to add a feature that would let codespell determine what a string delimiter is on a line by line basis, possibly limited to certain file patterns (extensions).

The detection would be on a line by line basis.

I understand that this could be somewhat tricky.

One way to implement it would be to have a regex that finds strings and replaces the delimiters of the string with spaces before looking for words.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Jan 23, 2024

Indeed, the default word regex includes ' in the word characters:

word_regex_def = r"[\w\-'’]+" # noqa: RUF001

The intent is to catch the apostrophe as part of words, not the quotes. However, since ' can be used as an apostrophe or a quote, processing quoted words fails. Perhaps we should strip("'") words, removing leading and trailing ' and keep occurrences inside the word.

@mdeweerd
Copy link
Contributor Author

I think that a regex like:

"""(["'])((?!\1)(?:\\\1|.)+?)(?=\1)"""

could do to match strings and remove the surrounding quotes before matching words.

https://regex101.com/r/hwVS2p/1

@DimitriPapadopoulos
Copy link
Collaborator

Wouldn't the above match non-word characters? Regexes are really a pain to read.

@DimitriPapadopoulos
Copy link
Collaborator

We need a regex that matches words, the above doesn't:

>>> import re
>>> 
>>> word_regex_def = """(["'])((?!\1)(?:\\\1|.)+?)(?=\1)"""
>>> word_regex = re.compile(word_regex_def)
>>> 
>>> word_regex.findall("""Some text with "errror" and 'errror'""")
 []
>>> 

Compare with the default regex r"[\w\-'’]+":

>>> import re
>>> 
>>> word_regex_def = r"[\w\-'’]+"
>>> word_regex = re.compile(word_regex_def)
>>> 
>>> word_regex.findall("""Some text with "errror" and 'errror'""")
 ['Some', 'text', 'with', 'errror', 'and', "'errror'"]
>>> 

@DimitriPapadopoulos DimitriPapadopoulos changed the title Detection of string delimiters . Detection of string delimiters Jan 24, 2024
@mdeweerd
Copy link
Contributor Author

mdeweerd commented Jan 24, 2024

The idea is to remove the quotes and then match the words.
Apparently the expression if found is flawed, I updated it.

The expression matches a quote ('"'), then ensures that if that quote can only occur in the string if it is escaped, and finishes on the same quote as it matched on.

The only thing is text within comments - there could be two single quotes that are not in strings.
I think the benefit outweighs that sideeffect and if it occurs there surely is a way to write the comment differently.
(Example in the last line of the test case).

#!python3
import re

test_str = ("// This is a \"comment\" and should be ignored.\n"
    "//This is also a 'comment' which should be ignored.\n"
    "printf(\"This is text meant to be \\\"captured\\\", and can include any type of character.\\n\");  // But \" must be escaped with a backslash.\n"
    "printf(\"This is the same as above, only with 'different' nested quotes.\\n\");\n"
    "putchar('I');\n"
    "putchar('\\'');\n"
    "printf(\"\"); printf(\"m thinking.\");  // First printf is not very useful.\n"
    "printf(\"\\\"OK!\\\"\");\n"
    "printf(\"Hello\"); printf(\" world!\\n\");\n"
    "printf(\"Hello\"); printf(\" world!\\n\");"
    "printf(\"%d file%s found.\\n\",iFileCount,((iFileCount != 1) ? \"s\" : \"\");\n"
    "printf(\"Result is: %s\\n\",sText);  // sText is \"success\" or \"failure\".\n"
    "return iReturnCode; // 0 ... \"success\", 1 ... \"error\"\n"
    "return iReturnCode; // It's been nice.  It's been great.\n"
)


pattern=re.compile(r"""(["'])((?:(?!\1)(?:\\\1|.))*?)\1""")

word_pattern = re.compile(r"[\w\-'’]+")

for line in test_str.splitlines():
    unquoted=pattern.sub(r" \2 ", line)
    print("ORG:"+line)
    print("NEW:"+unquoted)
    print(word_pattern.findall(unquoted))

Output which (the text after "NEW:") would be the input to the word matching logic:

ORG:// This is a "comment" and should be ignored.
NEW:// This is a  comment  and should be ignored.
['This', 'is', 'a', 'comment', 'and', 'should', 'be', 'ignored']
ORG://This is also a 'comment' which should be ignored.
NEW://This is also a  comment  which should be ignored.
['This', 'is', 'also', 'a', 'comment', 'which', 'should', 'be', 'ignored']
ORG:printf("This is text meant to be \"captured\", and can include any type of character.\n");  // But " must be escaped with a backslash.
NEW:printf( This is text meant to be \"captured\", and can include any type of character.\n );  // But " must be escaped with a backslash.
['printf', 'This', 'is', 'text', 'meant', 'to', 'be', 'captured', 'and', 'can', 'include', 'any', 'type', 'of', 'character', 'n', 'But', 'must', 'be', 'escaped', 'with', 'a', 'backslash']
ORG:printf("This is the same as above, only with 'different' nested quotes.\n");
NEW:printf( This is the same as above, only with 'different' nested quotes.\n );
['printf', 'This', 'is', 'the', 'same', 'as', 'above', 'only', 'with', "'different'", 'nested', 'quotes', 'n']
ORG:putchar('I');
NEW:putchar( I );
['putchar', 'I']
ORG:putchar('\'');
NEW:putchar( \' );
['putchar', "'"]
ORG:printf(""); printf("m thinking.");  // First printf is not very useful.
NEW:printf(  ); printf( m thinking. );  // First printf is not very useful.
['printf', 'printf', 'm', 'thinking', 'First', 'printf', 'is', 'not', 'very', 'useful']
ORG:printf("\"OK!\"");
NEW:printf( \"OK!\" );
['printf', 'OK']
ORG:printf("Hello"); printf(" world!\n");
NEW:printf( Hello ); printf(  world!\n );
['printf', 'Hello', 'printf', 'world', 'n']
ORG:printf("Hello"); printf(" world!\n");printf("%d file%s found.\n",iFileCount,((iFileCount != 1) ? "s" : "");
NEW:printf( Hello ); printf(  world!\n );printf( %d file%s found.\n ,iFileCount,((iFileCount != 1) ?  s  :   );
['printf', 'Hello', 'printf', 'world', 'n', 'printf', 'd', 'file', 's', 'found', 'n', 'iFileCount', 'iFileCount', '1', 's']
ORG:printf("Result is: %s\n",sText);  // sText is "success" or "failure".
NEW:printf( Result is: %s\n ,sText);  // sText is  success  or  failure .
['printf', 'Result', 'is', 's', 'n', 'sText', 'sText', 'is', 'success', 'or', 'failure']
ORG:return iReturnCode; // 0 ... "success", 1 ... "error"
NEW:return iReturnCode; // 0 ...  success , 1 ...  error 
['return', 'iReturnCode', '0', 'success', '1', 'error']
ORG:return iReturnCode; // It's been nice.  It's been great.
NEW:return iReturnCode; // It s been nice.  It s been great.
['return', 'iReturnCode', 'It', 's', 'been', 'nice', 'It', 's', 'been', 'great']

Edited: Add the word regex.

@DimitriPapadopoulos
Copy link
Collaborator

Isn't it much simpler to strip("'") the words matched by the default regex, asuggested in #3305 (comment)?

@mdeweerd
Copy link
Contributor Author

Isn't it much simpler to strip("'") the words matched by the default regex, asuggested in #3305 (comment)?

That would apply to all the "isn't", "shouldn't", "hasn't", etc .

@DimitriPapadopoulos
Copy link
Collaborator

How exactly do you fear it would apply? It seems to me that strip("'") does the right thing:

>>> "isn't".strip("'")
"isn't"
>>> 
>>> "'isn't'".strip("'")
"isn't"
>>> 

@mdeweerd
Copy link
Contributor Author

I admit I didn't think it through entirely, but here are some examples that could break the checks:

dont'->don't
dosent'->doesn't
gaus'->Gauss'
guas'->Gauss'
guass'->Gauss'
hasnt'->hasn't
havent'->haven't
isnt'->isn't
packges'->packages'
shouldnt'->shouldn't
thats'->that's
wasnt'->wasn't
wouldnt'->wouldn't
were'->we're

but some will have their equivalent without the quote, and some will not like "packges" and "were".
So stripping on words that are correctly spelled is ok, stripping words that are not, isn't.

@DimitriPapadopoulos
Copy link
Collaborator

You're right. We need to distinguish between:

  • you dont' say (dont'don't)
  • he said 'just dont' but he insisted (dont'don't')

Somehow I doubt we can achieve robust detection of single quotes in any file.

@mdeweerd
Copy link
Contributor Author

Strings in code would be mostly properly detected. Exceptions would be rare multiline strings that would probably have double quotes anyway.

Something like "He said 'just don't' ..." would be found only in comments.
( "... 'just dont' "would still be detected as a spelling issue ).

I think that:

  • It's rare to have multiple single quotes like that in comments;
  • It's incorrect to quote like that, so if detected, it can be corrected to "just don't";
  • It will be harder to robustly detect all cases, but currently a lot of strings with single quotes go unchecked.
    I think we admit that codespell is not the perfect spelling checker. But by handling the strings, codespell can detect more.

Further, the regex can be updated to strip 'just don't' to just don't by adding a negative lookbehind an lookahead (I did not test the regex, showing the principle):

r"""(?!<\w)(["'])((?:(?!\1)(?:\\\1|.))*?)\1(?!\w)"""

@DimitriPapadopoulos
Copy link
Collaborator

The thing is that we already have a regex to match words, which can be changed using option --regex. I am OK to post-process matched words, but I am reluctant to pre-process input before it is fed to the regex. It can be confusing. So you would need to provide a new default regex that does the quote filtering and word matching in a single pass.

@mdeweerd
Copy link
Contributor Author

So you would need to provide a new default regex that does the quote filtering and word matching in a single pass.

Seems quite challenging to me (the regex needs to match just the word, so as an extra challenge everything needs to go in lookbehind and lookahead expressions which have limitations).
Also, in that case it just needs to be passed to --regex .

So if that is the requirement (i.e., propose a regex to match the words), then I suggest to close this issue.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Jan 27, 2024

This just needs more effort to take into account backwards compatibility, and perhaps take into account a bigger picture. For example, text processing might include multiple steps, not only the word splitting step followed by the URL detection step. A new first step might somehow filter/preprocess the full text, before splitting into words. We need to think about the first step and make sure its scope is generalised, taken into account other possible purposes. A default first step and an option to modify it might be useful.

@mdeweerd
Copy link
Contributor Author

Ok, I tried to find a regex to replace the word extraction but it becomes quickly quite complex, long and timeconsuming to finetune. And 're' has limited support for lookbehind so I had to test with the regex module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants