search

raszi / node-tmp

Temporary file and directory creator for node.js
MIT License
732 stars 93 forks source link

Prefix and postfix - redundant dashes #256

Closed justanotheranonymoususer closed 3 years ago

justanotheranonymoususer commented 4 years ago

Tmp Version

0.2.1

Expected Behavior

When I use the following example code from the readme:

tmp.file({ mode: 0o644, prefix: 'prefix-', postfix: '.txt' }, /*...*/);

I expect to get a filename like this: prefix-blabla-blabla.txt That was the behavior in old versions.

Experienced Behavior

I get a filename like this: prefix--blabla-blabla-.txt

While the first redundant dash can be fixed by removing - from prefix, the dash before the extension stays there. i.e. If I have postfix: '.txt', I get [...]bla[...]-.tmp.

silkentrance commented 4 years ago

@justanotheranonymoususer nice find. Never thought of the suffix actually being meant for an file name extension. But since it is documented that way, we need to fix that.

My proposal would be to test for a leading dot .. If it is there, the hyphen - will be omitted. If it is not, then a hyphen will be included in the so generated file name.

What do you think?

justanotheranonymoususer commented 4 years ago

Works for me. But it's a special case, so how about the following more general solution:

Only add a dash after prefix if it ends with one of [a-zA-Z0-9]. Only add a dash before suffix if it begins with one of [a-zA-Z0-9].

This way, the example will be still valid, and the dash won't have to be removed from prefix as well.

On Sat, Jun 27, 2020 at 3:26 PM Carsten Klein notifications@github.com wrote:

@justanotheranonymoususer https://github.com/justanotheranonymoususer nice find. Never thought of the suffix actually being meant for an file name extension. But since it is documented that way, we need to fix that.

My proposal would be to test for a leading dot .. If it is there, the hyphen - will be omitted. If it is not, then a hyphen will be included in the so generated file name.

What do you think?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/raszi/node-tmp/issues/256#issuecomment-650554000, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMDRPFLE3Y562ORTVG6MN3RYXQPJANCNFSM4OJ7R42Q .

silkentrance commented 4 years ago

@justanotheranonymoususer would you care to provide us with a PR?

silkentrance commented 3 years ago

@justanotheranonymoususer I did not forget your issue. However, I am having a hard time with this, as the suffix is always meant to be a part of the filename and not representing its extension, e.g. .txt.

Apart from that, tmp is not meant to write out the terminal file. As of its nature, tmp will just write out temporary files. And if you need them to have a specific extension, feel free to use the available fs methods to move/rename an existing tmp file to a location where it gets persisted, once that it was finalized and closed.

Closing as invalid.

silkentrance commented 3 years ago

TODO fix the documentation.

silkentrance commented 3 years ago

@justanotheranonymoususer and since you are the only person who reported this, I think that we are safe with limiting the prefix use.