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

New: add option for camelcase (fixes #12527) #12528

Merged
merged 4 commits into from Nov 10, 2019
Merged

New: add option for camelcase (fixes #12527) #12528

merged 4 commits into from Nov 10, 2019

Conversation

g-plane
Copy link
Member

@g-plane g-plane commented Nov 5, 2019

What is the purpose of this pull request? (put an "X" next to item)

Changes an existing rule

What changes did you make? (Give an overview)
Added a new option ignoreImports to rule camelcase. The option is false by default.
Also, this PR is with some refactorings.

Fixes #12527 .

@g-plane g-plane 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 labels Nov 5, 2019
@g-plane g-plane self-assigned this Nov 6, 2019
@mdjermanovic
Copy link
Member

Should the option also allow later use of the imported name? For example:

import { snake_cased } from 'mod';

let x = snake_cased;

@g-plane
Copy link
Member Author

g-plane commented Nov 6, 2019

I'm not sure: online demo.

@mdjermanovic
Copy link
Member

That approach with ignoreDestructuring seems to be a recurring issue of confusion for users. I'm not 100% sure that it wasn't an unintentional oversight bug. It might be good to avoid the same problems with this option, of course only if it makes sense (though it might be strange that two similar options have a different logic).

If the option ignoreImports applies only to the import statement itself, that variable could be later only used to call it as a function?

@g-plane
Copy link
Member Author

g-plane commented Nov 6, 2019

I agree that this is really confusing, however, I'm also not sure whether it's a bug or not. For example, if we're using CommonJS:

const { kumiko_oumae } = require('./eupho')

let _ = kumiko_oumae

This is similar with ignoreImports.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mdjermanovic
Copy link
Member

I think it would be useful to clarify in the docs that the option skips only the import declaration itself, and that it doesn't additionally allow any particular use of the imported identifier later in the code, apart from what is already allowed by default (function calls) or by other options.

(if that's the intention, I'm not sure that an option which ignores only the declarations allows use case from the original issue?)

@ilyavolodin ilyavolodin merged commit 41b1e43 into master Nov 10, 2019
@ilyavolodin ilyavolodin deleted the issue-12527 branch November 10, 2019 01:16
@platinumazure
Copy link
Member

platinumazure commented Nov 11, 2019

@ilyavolodin Why was this merged? I don't believe either the PR or the related issue was accepted.

@mysticatea
Copy link
Member

I'm wondering if we should update our WIP checker bot checks the accepted label as well.

@kaicataldo
Copy link
Member

@mysticatea That sounds like a great idea

@mhchen
Copy link

mhchen commented Dec 23, 2019

I'm encountering this issue with the same problem that spawned this discussion (Apollo codegen generating variable names with underscores). I'm confused about the practicality of allowing the import but not allowing references to it later. If we have to rename it anyway (via const or let), is that any better than just renaming it in the import itself? (E.g. import { foo_snake as fooSnake })

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 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

Successfully merging this pull request may close these issues.

camelcase: Option to ignore imports
7 participants