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

Code clean up #1526

Merged
merged 4 commits into from Nov 11, 2017
Merged

Code clean up #1526

merged 4 commits into from Nov 11, 2017

Conversation

jomasti
Copy link
Contributor

@jomasti jomasti commented Nov 9, 2017

In the previous times I have made changes, I have noticed some duplication in code and wanted to deduplicate it. I have started that in this PR, and I wanted to open it up for some feedback. Is this desired? Should I go further?

Since the oldest supported version of `babel-eslint` (6.x) has the
key property on ClassProperty nodes, the workarounds aren't needed.
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks!

/**
* Checks if the PropTypes MemberExpression node passed in declares a required propType.
* @param {ASTNode} propTypeExpression node to check. Must be a `PropTypes` MemberExpression.
* @returns {Boolean} `true` if this PropType is required, `false` if not.
Copy link
Member

Choose a reason for hiding this comment

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

i think we can remove all these extra spaces; it's not aligned anyways

/**
* Checks if the Identifier node passed in looks like a defaultProps declaration.
* @param {ASTNode} node The node to check. Must be an Identifier node.
* @returns {Boolean} `true` if the node is a defaultProps declaration, `false` if not
Copy link
Member

Choose a reason for hiding this comment

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

also here

@jseminck
Copy link
Contributor

jseminck commented Nov 9, 2017

This is really great. There's a lot of duplicate code between prop-types and no-unused-prop-types rules that should be extracted out. I think this is a good first step. 👍

@EvHaus EvHaus self-requested a review November 11, 2017 06:40
Copy link
Collaborator

@EvHaus EvHaus left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks so much for the submission.

@ljharb ljharb merged commit dea04b6 into jsx-eslint:master Nov 11, 2017
@jomasti jomasti deleted the clean-up branch November 11, 2017 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants