Fix: support for MemberExpression with function body. #7400
Conversation
|
LGTM |
|
@sstern6, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @gyandeeps and @vitorbal to be potential reviewers. |
| return; | ||
| } | ||
|
|
||
| if (getParentNodeByType(node, "VariableDeclaration", "FunctionExpression")) { |
There was a problem hiding this comment.
Do we need to check for VariableDeclaration as well as VariableDeclarator? Please try removing this block and see if tests still pass.
| let parent = node.parent; | ||
|
|
||
| while (parent.type !== type && parent.type !== "Program") { | ||
| if (!stopAt) { |
There was a problem hiding this comment.
This block might not be necessary since we changed line 392 to check explicitly against Program, see if you can remove this?
|
LGTM |
|
@platinumazure Back up with your comments resolved |
nzakas
added
bug
|
@platinumazure want to take another look? |
|
Irritating, I could swear I reviewed this already. Sorry folks. LGTM, thanks @sstern6! |
|
I will merge.. just want feedback from our other indent experts like @not-an-aardvark @BYK |
| @@ -888,11 +880,11 @@ module.exports = { | |||
| // alter the expectation of correct indentation. Skip them. | |||
| // TODO: Add appropriate configuration options for variable | |||
| // declarations and assignments. | |||
| if (getVariableDeclaratorNode(node)) { | |||
| if (getParentNodeByType(node, "VariableDeclarator", "FunctionExpression")) { | |||
There was a problem hiding this comment.
I think this should check for ArrowFunctionExpression as well.
var foo = () => {
foo
.bar // <-- this needs to be checked
};There was a problem hiding this comment.
Agreed- the function could be adapted to take an array of stop node types.
There was a problem hiding this comment.
Will have this back up today!
Thanks for the feedback
There was a problem hiding this comment.
Sorry @platinumazure @not-an-aardvark used Array.includes in my solution but forgot it wasnt supported, reverted my changes and re pushed. Will have a solution up tonight. Just wanted to give you a heads up.
Thanks
|
LGTM |
|
LGTM |
|
LGTM |
|
LGTM |
|
@platinumazure @not-an-aardvark back up with your feedback to check for Looking forward to your feedback |
| * @param {array} nodeList list of node types passed in | ||
| * @returns {boolean} if parentType matches item in nodeList, otherwise false | ||
| */ | ||
| function checkIfParentTypeIsInStopNodeList(parentType, nodeList) { |
There was a problem hiding this comment.
This is a minor nit and shouldn't block this PR from being merged, but I think this function can be replaced with just nodeList.indexOf(parentType) !== -1.
There was a problem hiding this comment.
youre right, over engineered this. Be up in 1 min!
Thanks
There was a problem hiding this comment.
@not-an-aardvark implemented what you recommended, with a small tweak. With the current logic in the whole loop we would need to change it to nodeList.indexOf(parentType) === -1 because in the loop we are looping until parent === type, stopAtList.indexOf(parent.type) === 1, and parent.type === 'Program'. The semantics get a little tricky with the booleans and my implementation in the loop. Let me know if I need to explain further or I missed something.
Thank you
|
LGTM |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
This PR fixes: #7366 and possibly #7320.
What changes did you make? (Give an overview)
Updated indent.js and tests to ensure support for function bodies inside of MemberExpressions.
Is there anything you'd like reviewers to focus on?