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

Update: add propertiesPattern for id-match rule #10477

Closed
wants to merge 1 commit into from

Conversation

tinymins
Copy link
Contributor

@tinymins tinymins commented Jun 14, 2018

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

New feature: add new option propertiesPattern and it's errorMessage config to provide more humanized error report.

/* eslint id-match: [2, "^[a-zA-Z]+$", { 
  "propertiesPattern": "^[a-z_][a-zA-Z]*$",
  "errorMessage": "Identifier '{{name}}' is not in camelcase.",
  "propertiesErrorMessage": "Properties identifier '{{name}}' is not in lower camelcase."
}] */
const someObject = {
  _propertiesCanStartWithUnderscore: true,
};

Is there anything you'd like reviewers to focus on?

Temporarily not.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jun 14, 2018
@not-an-aardvark
Copy link
Member

Hi, thanks for the pull request!

Would you mind splitting this up into smaller PRs? It's generally easier to discuss and review proposed changes when they're not all bundled into the same PR, because this allows some of the changes to be merged incrementally. If there is disagreement about a particular feature of a large PR, then the entire PR ends up blocked, but if there is disagreement about a feature on one of several smaller PRs, then the other PRs can still get merged while that feature is discussed.

It would also be useful if you could fill out the bug report template for the bugs that you're fixing, and the rule change proposal template for the new options that you're proposing. At a glance, it seems like the fixes/options that this PR introduces could be worthwhile, but it's hard to tell because the motivation for these options isn't clear.

@tinymins
Copy link
Contributor Author

@not-an-aardvark Thanks, the reason why I merged all changes together is that the change is too large. If the PR got splitted into several parts, each one will get conflicted with any other one.

But if splitting is still recommended, I'll try my best start dealing with it now.

@platinumazure
Copy link
Member

Definitely please let's split this. If you're worried about conflicts, maybe you could just create PRs for the changes you think are most important/valuable first. Thanks!

@tinymins
Copy link
Contributor Author

I'll do this later, thanks!

@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 15, 2018
@tinymins
Copy link
Contributor Author

tinymins commented Aug 7, 2018

Hi, @not-an-aardvark @platinumazure, I've split codes into some smaller parts, and created a new PR #10554, could anyone got a review please?

@aladdin-add aladdin-add self-requested a review October 4, 2018 11:53
@tinymins tinymins force-pushed the id-match-property-pattern branch 5 times, most recently from c2f1b3f to ff79da4 Compare November 15, 2018 10:56
@nzakas
Copy link
Member

nzakas commented Dec 7, 2018

@tinymins sorry we lost track of this. What we're asking for is to split the bug fixes and the enhancements into separate pull requests so we can evaluate them separately. If you are willing to do that, we're happy to take a look at each of them. It's just too difficult to figure out which changes apply to which problems in this PR.

Thanks, and apologies again for losing track of this.

@kaicataldo
Copy link
Member

@tinymins One more friendly ping. Are you willing and able to continue working on this?

@tinymins
Copy link
Contributor Author

@tinymins One more friendly ping. Are you willing and able to continue working on this?

Sorry for the delay, I'll restart working on this as soon as possible.

@tinymins tinymins changed the title Update: fix several id-match bugs and add more custom options Update: add propertiesPattern for id-match rule May 28, 2019
@kaicataldo
Copy link
Member

kaicataldo commented May 28, 2019

No problem! Given that #10554 was merged, it's not clear to me what the remaining changes here will be. Before you start work on it, do you mind creating a new issue with a proposal of what changes you'd like to make? Once we accept that, then we can start looking at a PR.

Usually it would be fine to just update this PR, but given that it's been open for such a long time and it's changed quite a lot since it was first made, I think it would be clearer to start fresh. Making a new issue will hopefully also mean that it'll get a lot more visibility and discussion. Thanks!

@kaicataldo
Copy link
Member

Thanks for making a new issue! Can we close this PR now, and if the issue gets accepted, create a new PR? It’ll be a lot easier for everyone reviewing the issue and PR to follow what’s going on if we can separate from the previous proposal.

@ilyavolodin
Copy link
Member

Closing this PR based on the last comment. Please feel free to reopen it when the issue gets accepted.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Apr 23, 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 Apr 23, 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 enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants