Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add OPTION_ALLOW_SNAKE_CASE to variable name options #2383

Merged
merged 1 commit into from
Mar 24, 2017

Conversation

AJamesPhillips
Copy link
Contributor

@AJamesPhillips AJamesPhillips commented Mar 21, 2017

PR checklist

Overview of change:

Add OPTION_ALLOW_SNAKE_CASE to variable name options

CHANGELOG.md entry:

[new-rule-option] "allow-snake-case" option for `variable-name` rule (#2190)

Please let me know if there's anything else you need / would like to change. Many thanks.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @AJamesPhillips! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

LGTM. I just had one comment, but that's no blocker.

Unfortunately this conflicts with the not yet landed PR #2355. Applying your changes on top of the other PR should not be diffucult, though.

@@ -74,6 +76,16 @@ export class Rule extends Lint.Rules.AbstractRule {
class VariableNameWalker extends Lint.RuleWalker {
private shouldBanKeywords: boolean;
private shouldCheckFormat: boolean;
private get formatFailure(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need a getter for this, just do it once in the constructor

@AJamesPhillips
Copy link
Contributor Author

AJamesPhillips commented Mar 23, 2017

Thanks for the speedy response @ajafff . I made the change to the getter for formatFailure and then rebased onto 2355 but it's got all those "Abc committed with AJP..." commits. That ok or is there a preferred way of doing it like cherry-pick?

@ajafff
Copy link
Contributor

ajafff commented Mar 24, 2017

Your changes still LGTM after rebase.

got all those "Abc committed with AJP..." commits

I think that's ok for now. It would be nice if you can interactive rebase / cherry-pick on master once #2355 landed.

@AJamesPhillips
Copy link
Contributor Author

AJamesPhillips commented Mar 24, 2017 via email

@adidahiya
Copy link
Contributor

sorry for the delay; I have now merged #2355

@ajafff
Copy link
Contributor

ajafff commented Mar 24, 2017

github did not send a notification email for the last force push

ping @adidahiya this is ready to merge unless you have any objections

@adidahiya
Copy link
Contributor

thanks @AJamesPhillips!

@adidahiya adidahiya merged commit a39ad67 into palantir:master Mar 24, 2017
@AJamesPhillips AJamesPhillips deleted the allow_snake_case branch March 27, 2017 07:57
@ajafff ajafff mentioned this pull request Apr 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants