The former implementation had it's reason as well as the internal toString method is mostly not ideal. The main problem is that hasBuiltInToString does not know about the toPrimitive symbol. By checking for that, it would handle both cases right.

Thank you. Are you suggesting to modify it this way?

        case 115: { // 's'
          const tempArg = args[++a];
          if (typeof tempArg === 'number') {
            tempStr = formatNumberNoColor(tempArg, inspectOptions);
          } else if (typeof tempArg === 'bigint') {
            tempStr = formatBigIntNoColor(tempArg, inspectOptions);
          } else if (hasBuiltInToString(tempArg)) {
            tempStr = String(tempArg);
          } else {
            tempStr = inspect(tempArg, {
              ...inspectOptions,
              compact: 3,
              colors: false,
              depth: 0,
            });
          }
          break;
        }

@chenyuyang2022 no, the hasBuiltInToString method has to check for for the toPrimitive symbol as well. Please always run the tests as well. There should be test failures related to the current change.

@BridgeAR Thank you for the reminder. I have modified the implementation and passed all the tests with 'make test'.

@legendecas PTAL

Would you mind updating the hasBuiltInToString function instead as @BridgeAR suggested? Also, please add a test case in https://github.com/nodejs/node/blob/main/test/parallel/test-util-format.js as well. Thank you!

@legendecas Thank you, I've made the changes according to your suggestions. Could you please take a look?

@BridgeAR would you mind taking a look again?

CI: https://ci.nodejs.org/job/node-test-pull-request/57954/

@BridgeAR Would you mind reviewing it again?

@Ch3nYuY thank you for following up on the comments!

CI: https://ci.nodejs.org/job/node-test-pull-request/58665/

@Ch3nYuY seems like there is a linter error that has to be resolved before it's possible to land this

Thank you, @BridgeAR . I'll see how to solve it

@BridgeAR I've solved all the linter error. Could you please add the 'request-ci' tag again?

CI: https://ci.nodejs.org/job/node-test-pull-request/58873/

CI: https://ci.nodejs.org/job/node-test-pull-request/58920/

CI: https://ci.nodejs.org/job/node-test-pull-request/58925/

Hey @Ch3nYuY, the recent commits block the PR from being merged. Can you please rebase? :)

Thanks! @BridgeAR I resolved the merge conflicts and it should look ok now.

CI: https://ci.nodejs.org/job/node-test-pull-request/59110/

CI: https://ci.nodejs.org/job/node-test-pull-request/59152/

Landed in 7129915068b3613f8bb525c84ca4feaa7c9c2077 Sorry, wrong commit message!

Landed in dde2965

This actually broke the way Date is represented when using "%s". Prior to this commit, it would use ISO8601 format by way of utils.inspect(). After this commit, it appears to use toString() which has a radically different format.