Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Add react-prefer-private-members rule #95

Merged
merged 4 commits into from
Jun 4, 2018

Conversation

cartogram
Copy link
Contributor

Addresses one of the points here #77 by looking for React class components and making sure they do not have private members. React API stuff will pass through. Will fill in the docs once the general approach is approved.

@cartogram cartogram changed the title add react-prefer-private-members rule Add react-prefer-private-members rule May 20, 2018
@cartogram cartogram requested a review from lemonmade May 20, 2018 12:13
meta: {
docs: {
description: 'Disallow public members within React component classes',
category: 'Possible Errors',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure what to put in here. Just did my best to follow other rules in this repo.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably best practices

docs: {
description: 'Disallow public members within React component classes',
category: 'Possible Errors',
recommended: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same with this.

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

Approach looks great!

meta: {
docs: {
description: 'Disallow public members within React component classes',
category: 'Possible Errors',
Copy link
Member

Choose a reason for hiding this comment

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

It's probably best practices

} = node;
return {
node,
message: `${name} should be a private member of ${componentName}.`,
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to pass the variables?

].some((method) => method === name);
}

function isReactStaticProperty({key: {name}}) {
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 you probably want to allow all static properties, or at least those that are pascal-case, to allow for our (pretty common) pattern of exposing subcomponent as public static members.

parser: babelParser,
},
],
invalid: [
Copy link
Member

Choose a reason for hiding this comment

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

Need more tests than this (missing for private members, static members, etc

@cartogram cartogram force-pushed the react-prefer-private-members branch from 30510a3 to d5866f3 Compare May 28, 2018 02:07
@cartogram cartogram force-pushed the react-prefer-private-members branch from d5866f3 to 21378e0 Compare June 2, 2018 19:16
@cartogram
Copy link
Contributor Author

@lemonmade this is ready for another look. 🙏

}
```

The Life Cycle methods and static properties that are part of the React API are not required to be private for this rule.
Copy link
Member

Choose a reason for hiding this comment

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

lifecycle

```



Copy link
Member

Choose a reason for hiding this comment

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

So many empty lines :D

@cartogram cartogram force-pushed the react-prefer-private-members branch from 1f732c2 to db33276 Compare June 4, 2018 03:17
@cartogram cartogram merged commit 64ad5e7 into master Jun 4, 2018
@cartogram cartogram deleted the react-prefer-private-members branch November 13, 2018 14:38
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