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

New: Add grouped-accessor-pairs rule (fixes #12277) #12331

Merged
merged 4 commits into from Nov 22, 2019

Conversation

mdjermanovic
Copy link
Member

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

[X] New rule #12277

Please describe what the rule should do:

Checks object literals, class declarations and class expressions.

If a property has a getter and a setter, their definitions should be one right after the other.

Optionally, the rule can also enforce consistent order (getBeforeSet or setBeforeGet).

What category of rule is this? (place an "X" next to just one item)

[X] Enforces code style

Provide 2-3 code examples that this rule will warn about:

/*eslint grouped-accessor-pairs: "error"*/

var foo = {
    get a() {
        return this.val;
    },
    b: 1,
    set a(value) {
        this.val = value;
    }
};

class Foo {
    set a(value) {
        this.val = value;
    }
    b(){}
    get a() {
        return this.val;
    }
}
/*eslint grouped-accessor-pairs: ["error", "getBeforeSet"]*/

var foo = {
    set a(value) {
        this.val = value;
    },
    get a() {
        return this.val;
    }
};

class Foo {
    set a(value) {
        this.val = value;
    }
    get a() {
        return this.val;
    }
}

What changes did you make? (Give an overview)

Added new rule.

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

@mdjermanovic mdjermanovic added rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint labels Sep 28, 2019
Copy link
Sponsor Contributor

@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.

Could this rule be made to autofix, perhaps only in the absence of any computed or duplicate getters or setters?

docs/rules/grouped-accessor-pairs.md Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member Author

Could this rule be made to autofix, perhaps only in the absence of any computed or duplicate getters or setters?

I think it's possible for this rule to have autofix, in general. An issue with this could be comments around the code that is removed and around its new position, as the fixer can't know what are they related to.

Also, the fixer should check for a non-accessor duplicate between, because it 'breaks' the accessor property in the original code. Similar, a spread between could potentially do the same.

There are probably even more things to analyse further. Maybe to start without the autofix and add it later? :)

@mdjermanovic
Copy link
Member Author

Regarding the autofix, a getter/setter defined far below its pair could be a naming error. Perhaps better let the user see what's going on and fix it manually.

Could be safer to fix only the adjacent ones (switch positions if the order is incorrect).

@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 28, 2019

They can use git diffs to see that, just like any autofix that might mask a small error, but won't change the behavior.

@kaicataldo
Copy link
Member

I think it's fine to add autofixing later 👍

@aladdin-add
Copy link
Member

aladdin-add commented Nov 15, 2019

it is also allowed to define a getter/setter using Object.defineProperty() and Object.defineProperties(), is it in the scope of this PR?

@mdjermanovic
Copy link
Member Author

it is also allowed to define a getter/setter using Object.defineProperty() and Object.defineProperties(), is it in the scope of this PR?

It wasn't initially. I thought that this rule wouldn't provide much value in that case, since the getter and the setter for the same key are already "grouped" by being in the same property descriptor.

While thinking about it again, it can be useful when one wants to enforce the consistent order (e.g., get before set).

Maybe to add this as an option later? (it's a completely different code, like a rule within the rule).

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, thank you! Appreciate the thorough documentation and tests :)

@kaicataldo kaicataldo merged commit 312a88f into master Nov 22, 2019
@kaicataldo kaicataldo deleted the grouped-accessor-pairs branch November 22, 2019 18:02
@kaicataldo
Copy link
Member

Thanks for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants