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

add jsx-ignore option to no-magic-numbers rule #4460

Merged

Conversation

lizzzp1
Copy link
Contributor

@lizzzp1 lizzzp1 commented Jan 13, 2019

PR checklist

Overview of change:

Attempts to add an option to ignore jsx and leaves default as is.

Is there anything you'd like reviewers to focus on?

First PR in here, I may have missed some things. :)
Feedback appreciated.

CHANGELOG.md entry:

[new-rule-option] added jsx-ignore option to no-magic-numbers rule

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @lizzzp1! 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

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Exciting!, but needs some structural changes.

src/rules/noMagicNumbersRule.ts Outdated Show resolved Hide resolved
test/rules/no-magic-numbers/default/test.ts.lint Outdated Show resolved Hide resolved
src/rules/noMagicNumbersRule.ts Outdated Show resolved Hide resolved
src/rules/noMagicNumbersRule.ts Show resolved Hide resolved
@adidahiya adidahiya changed the title add jsx-ignore option add jsx-ignore option to no-magic-numbers rule Jan 14, 2019
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Looks like it's working! Some structural changes around option parsing, but the node/walking logic looks great.

src/rules/noMagicNumbersRule.ts Outdated Show resolved Hide resolved
src/rules/noMagicNumbersRule.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Just the one nitpick. Leaving this unedited if you want the satisfaction of applying the code change yourself 😄 otherwise I'll just make it & merge this in next week. Thanks!

src/rules/noMagicNumbersRule.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🙌 Thanks @lizzzp1! This looks great.

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.

3 participants