Update: Add considerPropertyDescriptor option to func-name-matching

[moeriki]

Fixes #9926.

What is the purpose of this pull request?

Change an existing rule.

What rule do you want to change?

func-name-matching

Does this change cause the rule to produce more or fewer warnings?

Fewer.

How will the change be implemented?

Add option { considerPropertyDescriptor: true } to have rule do some additional checks on what the function name should be.

Previously only property key was checked. With this option enabled it will check if the property is within Object.defineProperty, Object.defineProperties, or Object.create.

Please provide some example code that this change will affect:

/** eslint func-name-matching: ["always", { "considerPropertyDescriptor": true }] */
Object.defineProperty(foo, 'bar', {
  value: function baz() {}
})

What does the rule currently do for this code?

Function name `baz` should match property name `value`

What will the rule do after it's changed?

Function name `baz` should match property name `bar`

[eslintbot]

Thanks for the pull request, @moeriki! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[mention-bot]

@moeriki, thanks for your PR! By analyzing the history of the files in this pull request, we identified @annie, @ljharb and @not-an-aardvark to be potential reviewers.

[not-an-aardvark]

Thanks for the pull request! It looks like the issue that this addresses isn't accepted yet (it still has the "evaluating" label). We generally merge enhancements only if the team reaches consensus that they should be added (by having 3 👍s from team members on the issue, and having one team member willing to champion the proposal).

It's fine that you've submitted a pull request early -- I just wanted to let you know that it might take longer for this to get reviewed/merged, because the corresponding issue isn't accepted yet.

[kaicataldo]

This has been accepted. @moeriki Is there anything you need from us to finish this up?

[eslintbot]

Thanks for the pull request, @moeriki! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[moeriki]

Needed nothing but a nudge to pick this up again.

I'll rebase and add documentation.

[eslintbot]

LGTM

[moeriki]

Done.

[platinumazure]

Hi @moeriki, thanks for contributing and behalf of the team I would like to apologize for letting this fall through the cracks.

I've left one tiny nitpick, which isn't important.

I do have one change I'd like to request though, and that's this: Could you add a test with a simple object expression just to make sure that going up 4 levels in the AST doesn't crash?

The simplest case I can think of is this, but please do feel free to replace this if you come up with something better:

// valid
{
    code: "({ value: 42 })",
    options: ["always", { considerPropertyDescriptor: true }]
}

Fairly sure it will pass no problem, but it would be good to be absolutely sure.

[platinumazure]

Nit: s/wether/whether

[moeriki]

That's ok Kev. OS isn't easy.

• rebased on master
• fixed wether
• added test (and fixed TypeError)

        {
            code: "({ value: function value() {} })",
            options: [{ considerPropertyDescriptor: true }]
        }

I couldn't get it to crash on node.parent.parent.parent.parent; is ASTNode safe-guarded against this?

[j-f1]

is ASTNode safe-guarded against this?

I don’t think so. It looks like the FunctionExpression has four parents when an object literal is placed at the root of a program: FunctionExpression -> Property -> ObjectExpression -> ExpressionStatement -> Program -> null

[platinumazure]

Yeah, that's what I was thinking in my head too but I thought it was good to be sure. I couldn't come up with any case that could conceivably cause a crash.

…

On Dec 9, 2017 6:30 AM, "Jed Fox" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/rules/func-name-matching.js <#9078 (comment)>: > const includeModuleExports = options.includeCommonJSModuleExports; const ecmaVersion = context.parserOptions && context.parserOptions.ecmaVersion ? context.parserOptions.ecmaVersion : 5; + /** + * Check wether node is a CallExpression on Object. is ASTNode safe-guarded against this? I don’t think so. It looks like the FunctionExpression has four parents when an object literal is placed at the root of a program: FunctionExpression -> Property -> ObjectExpression -> ExpressionStatement -> Program -> null — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#9078 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWepk1u25O42SJCCYvj93SfzPNWW4Qks5s-n11gaJpZM4Ous_x> .

[moeriki]

Ok, I think it's good then.

1 review requesting changes by reviewers with write access.

I'm clicking through github interface but can't find where to confirm I did this :)

[platinumazure]

No worries, you don't need to do anything. That's my cue to go back and leave another review approving the current set of changes, which I'll do later today. Thanks for contributing and thanks again for your patience!

…

On Dec 9, 2017 8:43 AM, "Dieter Luypaert" ***@***.***> wrote: Ok, I think it's good then. 1 review requesting changes by reviewers with write access. I'm clicking through github interface but can't find where to confirm I did this :) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#9078 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWekoiRQK2PgnIjna1tL1z_eUOi2hoks5s-pypgaJpZM4Ous_x> .

[platinumazure]

LGTM, just want one or two other team members to review this.

[platinumazure]

Looks like our release-monitor check might have broken here. We shouldn't be in patch release mode right now (as of this comment).

@gyandeeps @not-an-aardvark Any ideas?

[not-an-aardvark]

That's odd, I'm not sure what happened there. I'll close and reopen this PR to make it create a new status.

[moeriki]

Docs updated.

[kaicataldo]

LGTM. Thanks for contributing to ESLint!

[platinumazure]

LGTM, thanks so much for your patience!

[platinumazure]

I'm going to let this sit for another day in case others wish to review. After that, I'll merge this. Thanks again for driving this through to completion, @moeriki, and I really hope we provide a better contribution experience next time.

[platinumazure]

Merged. Thanks for contributing!