[Refactor] Coming from#8673 (comment):

Instead of adding the overhead of the rest ofgetWrappingFixer(), suggestion: the specific text generation stuff should be extracted out and used here. Like, you could move the text generation from thegetWrappingFixer into a separate function, and use that separate helper here.

I think this means that we should extract codegen partfromgetWrappingFixer implementation to the separate utility function, not extract wholegetWrappingFixer call to the separate function. WDYT?

I think I misunderstood the explanation. thank you The code generation part that generates parentheses was separated and made into a function.

How come this test case gains parens? Is that just thegetWrappingFixer being overly cautions?

I also have this question. Feels like a bug to me?

At first, I thought it would be good for the expression to be wrapped, but as you said, I think it's excessive and looks like a bug.

However, in order to restore the test code, the getWrappingFixer function must be modified. Would it be a good idea to modify this function?

One thing worth noting is that this uses the parent node to infer whether parens are needed (seeif (isWeakPrecedenceParent(node))). So I'm wondering if that could give mistaken results for a scenario where we're reparenting code?

I understand that you are using the above node to infer whether parentheses are needed, but I don't understand this part (So I'm wondering if that could give mistaken results for a scenario where we're reparenting code?). Can you tell me in more detail what the scenario is for reparenting the code?

Sorry for taking a bit to get back to you - took me a while to figure out what I meant 😆 . Basically,getWrappingFixer() allows you to transform a node whose parent did not change.

// input<stuff before> nodeToBeModified <stuff after>// output<stuff before> modifiedNode <stuff after>

But,now that getWrappingCode has been extracted, it can be used instead like

// input<stuff before> nodeToBeModified <stuff after>// getWrappingCode outputmodifiedNode// used in fixer as<DIFFERENT stuff before> modifiedNode <DIFFERENT stuff after>

In fact, that's the whole reason that you've extracted it in this PR.

// inputcondition ?`${'left'||'right'}`.trim() :'lol';// getWrappingCodeOutput('left'||'right')// used in fixer ascondition ?('left'||'right').trim() :'lol';

So,getWrappingCode needs to deduce whether to wrap the expression in parentheses based on theNEW parent, not the existing one.

Used within thegetWrappingFixer those were the one and the same, so the lineif (isWeakPrecedenceParent(node)) was justified, butnow that getWrappingCode may be used indepdently, we'd need something likeif (isWeakPrecedenceNewParent(node, newParent)), in order for the function to give expected resultsin general.

I'd have to have a longer think to come up with test cases where this discrepancy would manifest, but hopefully this gets you started? If it's still unclear let me know and I can think up some test cases. The bottom line is that we need to be aware of whether parens are needed for thenew parent, not theexisting parent, in order forgetWrappingCode to be safe for general use outside ofgetWrappingFixer.

this might well be related actually:https://github.com/typescript-eslint/typescript-eslint/pull/8673/files/5a9f63085e1a3b08ae1c63e29deb34ca93b5165d#r1573616588

I apologize for not clear question. But the answers were the best!

If my understanding is correct, the parent that theisWeakPrecedenceNewParent function checks must be the new parent. In fact, the previous code was checking the parent of the expression, so it was always checking the templete literal node as the parent, so it was not getting normal results.

However, if i slightly modify the code to satisfy the above conditions, i will get the following results depending on the conditions of innerNode and parents condition.

// beforetrue ? `${'test' || ''}`.trim() : undefined;// aftertrue ? (('test' || '')).trim() : undefined;

In my opinion, it seems better to use parentheses when both the logic of applying parentheses to innerNode and the logic of examining the state of the parent are satisfied. so i revertgetWrappingFixer and make newgetWrappingCode function

I think we're just getting started, so i appreciate your feedback.

Sorry for taking a bit to get back to you - took me a while to figure out what I meant 😆 . Basically,getWrappingFixer() allows you to transform a node whose parent did not change.

// input<stuff before> nodeToBeModified <stuff after>// output<stuff before> modifiedNode <stuff after>

But,now that getWrappingCode has been extracted, it can be used instead like

// input<stuff before> nodeToBeModified <stuff after>// getWrappingCode outputmodifiedNode// used in fixer as<DIFFERENT stuff before> modifiedNode <DIFFERENT stuff after>

In fact, that's the whole reason that you've extracted it in this PR.

// inputcondition ?`${'left'||'right'}`.trim() :'lol';// getWrappingCodeOutput('left'||'right')// used in fixer ascondition ?('left'||'right').trim() :'lol';

So,getWrappingCode needs to deduce whether to wrap the expression in parentheses based on theNEW parent, not the existing one.

Used within thegetWrappingFixer those were the one and the same, so the lineif (isWeakPrecedenceParent(node)) was justified, butnow that getWrappingCode may be used indepdently, we'd need something likeif (isWeakPrecedenceNewParent(node, newParent)), in order for the function to give expected resultsin general.

I'd have to have a longer think to come up with test cases where this discrepancy would manifest, but hopefully this gets you started? If it's still unclear let me know and I can think up some test cases. The bottom line is that we need to be aware of whether parens are needed for thenew parent, not theexisting parent, in order forgetWrappingCode to be safe for general use outside ofgetWrappingFixer.

Let's keep the parent deduced by thenode... so,

functionisWeakPrecedenceParent(node:TSESTree.Node):boolean{constparent=node.parent;if(!parent){returnfalse;}

What do you think of reframing this function to look something like this?

/** * Useful jsdoc here */exportfunctiongetMovedNodeCode(params:{sourceCode:Readonly<TSESLint.SourceCode>;nodeToMove:TSESTree.Node;destinationNode:TSESTree.Node;}):string{const{ sourceCode,nodeToMove:existingNode, destinationNode}=params;constcode=sourceCode.getText(existingNode);if(isStrongPrecedenceNode(existingNode)){// Moved node never needs parensreturncode;}if(!isWeakPrecedenceParent(destinationNode)){// Destination would never needs parens, regardless what node moves therereturncode;}// Parens may be necessaryreturn`(${code})`;}

It looks much more intuitive and good. We will reflect it appropriately

(nit) the namewrappingCode is slightly stale with the rename, but I don't have many good ideas for a better name 🙃. Any ideas?codeToReplaceTemplate? Up to you if you want to change this or leave it 🤷‍♂️

Since the function name is getMovedNodeCode, how about changing the variable name to movedNodeCode? If you don't think it's appropriate, it might be a good idea to keep the current name.

Makes sense to me 👍

[Praise] Great reuse of the existing functions!