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

mdjermanovic commented Sep 28, 2019

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?

Copy link
Contributor

ljharb left a comment

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

mdjermanovic commented Sep 28, 2019

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

mdjermanovic commented Sep 28, 2019

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

kaicataldo commented Sep 28, 2019

I think it's fine to add autofixing later 👍

mdjermanovic and others added 3 commits Sep 28, 2019
@mdjermanovic mdjermanovic force-pushed the grouped-accessor-pairs branch from bc5102a to 5896421 Oct 3, 2019
@aladdin-add aladdin-add self-requested a review Nov 13, 2019
@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

mdjermanovic 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?

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 left a comment

LGTM, thank you! Appreciate the thorough documentation and tests :)

@kaicataldo kaicataldo merged commit 312a88f into master Nov 22, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191115.10 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo kaicataldo deleted the grouped-accessor-pairs branch Nov 22, 2019
@kaicataldo
Copy link
Member

kaicataldo commented Nov 22, 2019

Thanks for contributing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.