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

Rule camelcase cannot detect between upper-camelcase and lower-camelcase when lint property. #10473

Closed
tinymins opened this issue Jun 13, 2018 · 15 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@tinymins
Copy link
Contributor

tinymins commented Jun 13, 2018

What rule do you want to change?

camelcase

Does this change cause the rule to produce more or fewer warnings?

More warnings.

How will the change be implemented? (New option, new default behavior, etc.)?

New options.

{
    propertiesStyle: "upper" | "lower" | "all",
}

Please provide some example code that this change will affect:

This is bad code example.

const camel = {};
camel.UpperCamelcase = true;

This is good code example.

const camel = {};
camel.lowerCamelcase = true;

What does the rule currently do for this code?

All will be ok. The rule currently do not detect what type of camelcase is using for property.

What will the rule do after it's changed?

There will be an option to configure upper-camelcase or lower-camelcase for property.

What's more

I'm willing to fork and make a PR if my idea is acceptable. 😃

https://github.com/tinymins/eslint/tree/properties-style

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jun 13, 2018
@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 13, 2018
@tinymins tinymins changed the title Rule camelcase cannot detect between big-camelcase and little-camelcase when lint property. Rule camelcase cannot detect between upper-camelcase and lower-camelcase when lint property. Jun 13, 2018
@platinumazure
Copy link
Member

Can id-match be used in this case? (Legitimately not sure and wanted to ask.)

@tinymins
Copy link
Contributor Author

@platinumazure I'm sorry that I've reply to wrong thread just now. Actually id-match has problems when limit lower camelcase for object property name.

@tinymins
Copy link
Contributor Author

image
@platinumazure Here's the screenshot. Use id-match in current version cannot limit lower-camelcase in property. Rule id-match also need to be updated to satisfy this situation.

@tinymins
Copy link
Contributor Author

Or should I turn to concentrate on fix id-match bugs?

@platinumazure
Copy link
Member

No, I was only asking if it could work for your use case.

Let's see what the rest of the team thinks.

@tinymins
Copy link
Contributor Author

tinymins commented Jun 13, 2018

Thanks, I've tried that.

Only if:

  • id-match rule can separate object property pattern from common pattern
  • id-match rule will ignore embedded javascript object names such as Object, Array, Boolean

It could work for my use case.

@not-an-aardvark
Copy link
Member

My understanding is that capital letters at the start of a name are conventionally used for constructor functions, whereas lowercase letters are conventionally used for almost everything else. If this option were implemented, do you think you would use lower-camelcase letters for constructor names? To me, it seems like the problem is that ESLint is allowing upper-camelcase names for non-constructor-function values, not that it's allowing upper-camelcase names in general. (Unfortunately, the former problem probably wouldn't be fixable due to dynamic typing.)

@tinymins
Copy link
Contributor Author

@not-an-aardvark Yes, just like what you said, upper-camelcase means it is a constructor.
But constructor usually does not appears in an object.

// We usually use constructor like this:
import ClassA from 'class-a';
const a = new ClassA();

// Hardly ever seen this:
// obj is defined somewhere
const a = obj.ClassA();

So we want to lint object properties name in lower-camelcase.

Thanks a lot for reply.

@tinymins
Copy link
Contributor Author

tinymins commented Jun 13, 2018

It seems that I should stop thinking about modify rule camelcase and should be concentrated on fixing id-match bugs?

@tinymins
Copy link
Contributor Author

tinymins commented Jun 14, 2018

Hi everyone, I'v forked and made some improvements for rule id-match. Several bugs has been fixed in the days just past. I think maybe we should remove underscores ignore codes in camelcase.

Thanks everyone for keeping focus on this issue, here's my new PR: #10477

@mcclowes
Copy link

mcclowes commented Jul 4, 2018

Hey @tinymins, will you have a chance to split your PR (#10477) soon? Would be good to have this resolved! :)

@tinymins
Copy link
Contributor Author

tinymins commented Jul 4, 2018

Sorry that I got busy on my work and had a cold last week. I'm working on it now. @mcclowes

@tinymins
Copy link
Contributor Author

tinymins commented Jul 4, 2018

New PR created, I don't know if there will be a conflict. If there is, I'll do a git rebase job. @mcclowes

@tinymins
Copy link
Contributor Author

tinymins commented Jul 4, 2018

#10554

@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 10, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion auto closed The bot closed this issue enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

5 participants