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

feat: Add require-read-only-array rule (#236) #279

Merged
merged 3 commits into from
Oct 6, 2017

Conversation

pnevyk
Copy link
Contributor

@pnevyk pnevyk commented Oct 1, 2017

This rule enforces use of $ReadOnlyArray instead of Array or array
shorthand notation.

This rule enforces use of `$ReadOnlyArray` instead of `Array` or array
shorthand notation.
@pnevyk
Copy link
Contributor Author

pnevyk commented Oct 1, 2017

Well, right after I submitted this PR I noticed there is already one solving this issue. I am so sorry for this duplicate, it should be probably closed. Next time, I will pay more attention.

@pnevyk pnevyk closed this Oct 1, 2017
@gajus
Copy link
Owner

gajus commented Oct 2, 2017

@pnevyk If you do not mind, I prefer your PR.

@gajus gajus reopened this Oct 2, 2017
@pnevyk
Copy link
Contributor Author

pnevyk commented Oct 2, 2017

Ok, that's fine. And is there already a decision in points you noted in your PR (name no-array and detection of mutation in scope)?

@gajus
Copy link
Owner

gajus commented Oct 2, 2017

detection of mutation in scope

No need. If user intends to perform a mutation, he will just suppress the warning. Otherwise, it is easy to perform accidental mutation without even being aware of this rule.

As for the rule name, "no-array" is a bit too generic. I think "no-mutable-array" would work the best.

@pnevyk
Copy link
Contributor Author

pnevyk commented Oct 5, 2017

@gajus So I renamed the rule to no-mutable-array. Is there anything to be done yet?

@gajus gajus merged commit 8a08fcd into gajus:master Oct 6, 2017
gajus added a commit that referenced this pull request Oct 6, 2017
@gajus
Copy link
Owner

gajus commented Oct 6, 2017

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants