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

💅 useNamingConvention can suggest useless fix #1786

Closed
1 task done
xnuk opened this issue Feb 11, 2024 · 10 comments · Fixed by #1806
Closed
1 task done

💅 useNamingConvention can suggest useless fix #1786

xnuk opened this issue Feb 11, 2024 · 10 comments · Fixed by #1806
Assignees
Labels
A-Linter Area: linter S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@xnuk
Copy link

xnuk commented Feb 11, 2024

Environment information

Used biome-linux-x64 binary from the release (sha256sum: adf8a6029f43ac6eb07c86519f7ff08875915acec082d0be9393888044806243)

CLI:
  Version:                      1.5.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           linux

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "foot"
  JS_RUNTIME_VERSION:           unset
  JS_RUNTIME_NAME:              unset
  NODE_PACKAGE_MANAGER:         unset

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

Rule name

lint/style/useNamingConvention

Playground link

https://biomejs.dev/playground/?lintRules=all&code=ZQB4AHAAbwByAHQAIABjAG8AbgBzAHQAIABkAGEAdABhACAAPQAgAHsAIABIxVWxWNU4wZTGOgAgADEAIAB9AAoA

Expected result

Currently it shows this:

   This object property name should be in camelCase.
  
  > 1  export const data = { 안녕하세요: 1 }
                             ^^^^^^^^^^
    2  
  
   The name could be renamed to `안녕하세요`.

Since the suggested fix is same as original, the message should be removed or replaced to something else. Disabling the lint could be an option.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@nhedger nhedger self-assigned this Feb 11, 2024
@ematipico ematipico added A-Linter Area: linter S-Bug-confirmed Status: report has been confirmed as a valid bug labels Feb 11, 2024
@ematipico
Copy link
Member

I suppose we should ignore identifiers that aren't UTF-8

@nhedger
Copy link
Member

nhedger commented Feb 11, 2024

I suppose we should ignore identifiers that aren't UTF-8

AFAIK, Korean characters can be represented as UTF-8.

Not 100% sure, but most likely concepts such as camel casing and such only apply to latin scripts.

@nhedger nhedger removed their assignment Feb 11, 2024
@xnuk
Copy link
Author

xnuk commented Feb 11, 2024

Note that Koreans mostly do not prefer to write variable names in Korean, except when they have to communicate with outside services, written by who prefers to do (mostly government ones).

@nhedger
Copy link
Member

nhedger commented Feb 11, 2024

@ematipico I was thinking we could change the return type of Case::identify from Case to Option<Case>. This would allow us to represent the fact that there is no case for the given identifier. How does that sound?

@ematipico
Copy link
Member

I'm not very familiar with the rule unfortunately

@Conaclos
Copy link
Member

I was thinking we could change the return type of Case::identify from Case to Option. This would allow us to represent the fact that there is no case for the given identifier. How does that sound?

Case has already an Unknown variant.
Case::identify returns Case::idemtify in two cases:

  1. when the string mix several cases, e.g UPPER_lower
  2. when the string uses characters that cannot be in uppercase/lowercase and are not - or _.

I am not sure how we should handle this.
One idea: we could treat letters that cannot be in lowercase and uppercase as being both in lowercase and uppercase. This will require adding a new variant Case::LowerUpper.

@Conaclos Conaclos self-assigned this Feb 12, 2024
@nhedger
Copy link
Member

nhedger commented Feb 12, 2024

My reasoning was that these scripts might not have a case at all, so giving them a case variant of LowerUpper, or any case for that matter seems odd.

@Conaclos
Copy link
Member

Conaclos commented Feb 12, 2024

My reasoning was that these scripts might not have a case at all, so giving them a case variant of LowerUpper, or any case for that matter seems odd.

I didn't check the implantation of the eslint rule, however the rule seems to treat these characters as both uppercase and lowercase character.

Otherwise, what do you suggest? What about mixing latin characters with such characters?

EDIT: We could create a new case named Unicase. This case could require all characters to be alphanumeric characters that cannot be neither in lowercase nor in uppercase (numbers or syllabary). This case could be accepted everywhere. Any mix of latin and syllabary characters could result in the unknown case that is rejected.
For example:

export const data = {
  안녕하세요: 1, // accepted
  안_녕_하_세_요: 2, // rejected
  a안b녕c하d세e요f: 2, // rejected
} 

@xnuk Could this change fulfills your needs?

@xnuk
Copy link
Author

xnuk commented Feb 13, 2024

export const data = {
  안녕하세요: 1, // accepted
  안_녕_하_세_요: 2, // rejected
  a안b녕c하d세e요f: 2, // rejected
} 

@xnuk Could this change fulfills your needs?

Seems okay for now, as long as Biome does not suggest useless fix for 안_녕_하_세_요.

+ Korean has word spacing, so for example 예금은행_수신금리_신규취급액_기준코드 should be accepted to 'make sense', but, again, coding in Korean is still a minor opinion.

@Conaclos
Copy link
Member

If some users request it, we could add a new case for unicase characters with underscores.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants