Skip to content
This repository was 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

@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

Sorry, something went wrong.

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

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

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!

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.

None yet

3 participants