[Refactor] Coming from #8673 (comment):

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

I think this means that we should extract codegen part from getWrappingFixer implementation to the separate utility function, not extract whole getWrappingFixer 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 the getWrappingFixer 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 (see if (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 output
modifiedNode
// 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.

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

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

Used within the getWrappingFixer those were the one and the same, so the line if (isWeakPrecedenceParent(node)) was justified, but now that getWrappingCode may be used indepdently, we'd need something like if (isWeakPrecedenceNewParent(node, newParent)), in order for the function to give expected results in 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 the new parent, not the existing parent, in order for getWrappingCode to be safe for general use outside of getWrappingFixer.

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 the isWeakPrecedenceNewParent 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.

// before
true ? `${'test' || ''}`.trim() : undefined;
// after
true ? (('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 revert getWrappingFixer and make new getWrappingCode 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 output
modifiedNode
// 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.

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

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

Used within the getWrappingFixer those were the one and the same, so the line if (isWeakPrecedenceParent(node)) was justified, but now that getWrappingCode may be used indepdently, we'd need something like if (isWeakPrecedenceNewParent(node, newParent)), in order for the function to give expected results in 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 the new parent, not the existing parent, in order for getWrappingCode to be safe for general use outside of getWrappingFixer.

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

function isWeakPrecedenceParent(node: TSESTree.Node): boolean {
  const parent = node.parent;
  if (!parent) {
    return false;
  }

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

/**
 * Useful jsdoc here
 */
export function getMovedNodeCode(params: {
  sourceCode: Readonly<TSESLint.SourceCode>;
  nodeToMove: TSESTree.Node;
  destinationNode: TSESTree.Node;
}): string {
  const { sourceCode, nodeToMove: existingNode, destinationNode } = params;
  const code = sourceCode.getText(existingNode);
  if (isStrongPrecedenceNode(existingNode)) {
    // Moved node never needs parens
    return code;
  }

  if (!isWeakPrecedenceParent(destinationNode)) {
    // Destination would never needs parens, regardless what node moves there
    return code;
  }
  
  // Parens may be necessary
  return `(${code})`;
}

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

(nit) the name wrappingCode 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!