path: add dot for path.format if absent in ext

[theanarkh]

Fixes: #44343

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[tniessen]

This does not look like a regression, so changing the behavior is probably at least semver-major (thus cc @nodejs/tsc).

Some existing application could be relying on the current behavior:

const editedImageFileName = path.format({
  name: path.basename(originalImagePath, '.png'),
  ext: '-new.png'
});

[BridgeAR]

I am +1 as long as not many rely upon the behavior. Let's run CITGM and check that.

We should also update the documentation to include an example.

[mcollina]

lgtm

[tniessen]

I am +1 as long as not many rely upon the behavior. Let's run CITGM and check that.

We could also employ a deprecation cycle for the current behavior with a runtime warning, then change the behavior and remove the warning.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[theanarkh]

The label of this PR is semver-major, can it be merged directly ?

[tniessen]

The label of this PR is semver-major, can it be merged directly ?

semver-major PRs need at least two TSC approvals and a CitGM run.

[mitul1719]

Maybe we can add an extra parameter as path.format({...,format:true}) ,if the format key is specified ,in path.format ,it will add the '.' automatically ,else it will use the previous version. this way backward compatibility can be reinforced!

[tniessen]

Maybe we can add an extra parameter as path.format({...,format:true}) ,if the format key is specified ,in path.format ,it will add the '.' automatically ,else it will use the previous version. this way backward compatibility can be reinforced!

I don't think adding an additional option would help here if it needs to be added explicitly. If the user is willing to modify their code (e.g., to add a new option) and they know that their extension does not start with a ., they might as well do this:

path.format({ ext: `.${someExtension}` })

(Or a slightly longer version if someExtension may or may not start with a .)

[targos]

Please add a "changes" entry to the documentation

[theanarkh]

Please add a "changes" entry to the documentation

Done, thanks !

[aduh95]

Maybe we can add an extra parameter as path.format({...,format:true}) ,if the format key is specified ,in path.format ,it will add the '.' automatically ,else it will use the previous version. this way backward compatibility can be reinforced!

I don't think adding an additional option would help here if it needs to be added explicitly.

We could add an option to land this as semver-minor PRs that contain new features and should be released in the next minor version. and switch its default value in a followup semver-major PRs that contain breaking changes and should be released in the next major version. PR, that would also allow folks to opt-out of the new behavior. No strong feeling from my side either way, I just wanted to point out that's also a possible solution.

resolved

[aduh95]

@nodejs/tsc this needs at least one more approving review to land.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 22c39b1

[mscdex]

Did this ever get a CITGM run?

[theanarkh]

@mscdex I think, no. can we need to revert it ?

[theanarkh]

@aduh95 Hi, Can you take a look at this ? thanks !

Did this ever get a CITGM run ?

[aduh95]

After the fact CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3025/
CITGM(v18.11.0): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3015/

It probably won't give us anything interesting at this point, sorry I missed the CITGM requests before landing though.