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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Refactor to use messageId in a number of rules #12715

Merged
merged 2 commits into from Jan 7, 2020
Merged

Chore: Refactor to use messageId in a number of rules #12715

merged 2 commits into from Jan 7, 2020

Conversation

bradzacher
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[x] Other: Helping to bring the codebase closer to standardisation.

What changes did you make? (Give an overview)

I went through the rules and added messages to a number of older rules.
I picked non-deprecated rules, and rules that could be simply changed (i.e. essentially just a prop change in context.report).

I did as many as I could before getting bored 馃槄 (there's about 71 more rules to go)

There should be no functionality changes at all from this PR.
No new tests have been added, and no tests have been changed (apart from switching from hardcoded message to messageId + data).
Whilst I was at it, I did a little bit of reformatting to standardise on the format style I saw in other rules.

Let me know if you want me to split this up into separate PRs because of its size.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Dec 29, 2019
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kaicataldo kaicataldo added chore This change is not user-facing and removed triage An ESLint team member will look at this issue soon labels Jan 3, 2020
Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

lib/rules/no-invalid-regexp.js Show resolved Hide resolved
tests/lib/rules/no-restricted-globals.js Show resolved Hide resolved
Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this!

@ilyavolodin ilyavolodin merged commit 9dfc850 into eslint:master Jan 7, 2020
@bradzacher bradzacher deleted the message-ids branch January 8, 2020 05:18
@G-Rath
Copy link
Contributor

G-Rath commented Jan 31, 2020

I did as many as I could before getting bored 馃槄 (there's about 71 more rules to go)

@kaicataldo @ilyavolodin I don't mind tackling the rest of them (or at least until I've gotten board as well), as it would give me a good excuse to rummage around carefully & respectively explore the codebase.

Could be a good bullet point for the v7 release if they all got done in time?

@kaicataldo
Copy link
Member

Great! Let us know if we can help with anything.

Could be a good bullet point for the v7 release if they all got done in time?

I don鈥檛 see why not! Any PRs that are merged are always listed in the release notes blog post as well :)

@G-Rath G-Rath mentioned this pull request Feb 1, 2020
2 tasks
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 7, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants