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

mdjermanovic commented Nov 6, 2019

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

mdjermanovic commented Nov 6, 2019

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.

g-plane added 2 commits Nov 6, 2019
Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@mdjermanovic
Copy link
Member

mdjermanovic commented Nov 7, 2019

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
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191107.14 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@ilyavolodin ilyavolodin deleted the issue-12527 branch Nov 10, 2019
@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

mysticatea commented Nov 11, 2019

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

@kaicataldo
Copy link
Member

kaicataldo commented Nov 25, 2019

@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.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.