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

Ordinal number in identifier classified as typo #466

Open
foriequal0 opened this issue Apr 25, 2022 · 3 comments
Open

Ordinal number in identifier classified as typo #466

foriequal0 opened this issue Apr 25, 2022 · 3 comments
Labels
A-token Area: tokenization, including definition of identifiers and words C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@foriequal0
Copy link
Contributor

#410 seems to fix only 2nd__ or __3rd at the identifier tokenizer level.
Variable names such as level_2nd are split as ["level", "2", "nd"] by the SplitIdent.
I think I can fix it by marking words whether they might be ordinal suffixes or not and then using a different dictionary according to that.

@epage epage added the C-bug Category: bug label Apr 25, 2022
@epage
Copy link
Collaborator

epage commented Apr 25, 2022

So far we've focused our ordinal handling on english, which extended to markdown support. We have not tried to handle ordinals within variable names.

Doing this would require special casing our case splitter for ordinals. I'm a bit mixed on the value of this; personally I've rarely seen ordinals in variable names and would probably discourage it if I saw it in a review, independent of typos behavior.

@epage epage added C-enhancement Category: Raise on the bar on expectations and removed C-bug Category: bug labels Apr 25, 2022
@neiljp
Copy link

neiljp commented Mar 13, 2023

From #681, in my case these are not variable names, but rather test-case id strings which are being formatted in a snake-case-like way (partly since pytest uses - as a test-case id-separator).

I agree that with variable names I would recommend expanding them if I saw them as one-offs, and if multiple may query their presence.

However, here these are test case names, and eg. 1st/2nd/3rd is more concise and easier to read than first/second/third due to the digits present.

@epage
Copy link
Collaborator

epage commented Mar 22, 2023

FYI #695 provides a new workaround for false positives

@epage epage added A-token Area: tokenization, including definition of identifiers and words S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing S-waiting-on-decision Status: Waiting on a go/no-go before implementing and removed S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Feb 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-token Area: tokenization, including definition of identifiers and words C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

3 participants