Skip to content

When using the template option tmp does not join paths with opts.dir or tmpDir if no path is given in the template #143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
8 tasks
silkentrance opened this issue Jul 11, 2017 · 6 comments · Fixed by #144

Comments

@silkentrance
Copy link
Collaborator

silkentrance commented Jul 11, 2017

Operating System

  • Linux
  • Windows 7
  • Windows 10
  • MacOS
  • other: all

NodeJS Version

  • 0.x
  • 4.x
  • 6.x
  • 7.x
  • other: all

Tmp Version

All since the time the template option was introduced up to 0.31

Expected Behavior

When passing in the template option, tmp should join paths with either the dir option or the default tmpDir if the template does not already contain a path.

Experienced Behavior

Temporary files and directories will be created in the CWD. In addition, both absolute and relative paths passed as part of the template will not be rejected.

See also

#141

@silkentrance silkentrance changed the title When using the template option tmp does not join paths with opts.dir or tmpDir When using the template option tmp does not join paths with opts.dir or tmpDir and accepts arbitrary paths in the template Jul 11, 2017
@silkentrance
Copy link
Collaborator Author

silkentrance commented Jul 11, 2017

On second thought, we should elaborate on the template thing a bit. E.g. sprintf like functionality allowing for datetime/random chars. As it stands now, template is actually the same as a combination of the following options: dir, prefix, postfix and a random count, where count is limited to 6.

Perhaps we should also introduce a length option that allows the user to control the amount of random chars appended to the prefix during normal non template operation instead of the fixed/default 12 random chars as it is now the case.

And when using template, one should have the option to

"{dt:YYYYMMDD-HHMMSS}-{######}"
"{pid}..."

or even

"{##}-{dt:YYYY}-{###}-{pid}"

if one would ever need such a construct

where {pid} is a place holder for the current process id and {dt:format} is a place holder for the current datetime with an optional user provided format and {#...} stands for a number of random chars.

and so on. What do you think of this instead of merely limiting template to a prefix/random/postfix?

@raszi
Copy link
Owner

raszi commented Jul 13, 2017

I am not sure I like this change. First of all this is a breaking change, second this will also break the behavior of the template option, since it would not behave as mkstemp does as it is advertised in the README.

Although the idea of having a more generic templating sounds nice at the first thought but it would lead too far and I am not sure that it would be widely used. If somebody needs something like that then they could define their own and then pass the generated path as the template.

But I am open for further discussion about it.

@silkentrance
Copy link
Collaborator Author

silkentrance commented Jul 14, 2017

@raszi Okay, I can see that backwards compatibility is an issue here.

How about then checking whether template resembles a relative or absolute path? If not, we would append opts.dir || tmpDir instead?

As for the extensions to the template: sure, this can easily be accomplished by an external templating system. How about then allowing the user to pass in a callable as a template? Which, upon invocation, would return the rendered string? We could be passing the random char generator as input to that callable as well as opts.dir || tmpDir, e.g.

tmp.tmpName({template: function (tmpdir, randomGenerator) { ... }}); 

This way we remain backwards compatible and provide for future extension.

@silkentrance silkentrance changed the title When using the template option tmp does not join paths with opts.dir or tmpDir and accepts arbitrary paths in the template When using the template option tmp does not join paths with opts.dir or tmpDir if no path if given in the template Jul 14, 2017
@silkentrance
Copy link
Collaborator Author

I have adjusted the description and summary.

@silkentrance silkentrance changed the title When using the template option tmp does not join paths with opts.dir or tmpDir if no path if given in the template When using the template option tmp does not join paths with opts.dir or tmpDir if no path is given in the template Jul 16, 2017
@silkentrance
Copy link
Collaborator Author

silkentrance commented Jul 18, 2017

@raszi The mkstemps like 'template system' is not compatible with the Windows platform and is very failure prone for users, especially when using hard coded paths.

See https://ci.appveyor.com/project/raszi/node-tmp/build/1.0.132/job/6cln6y1vlxxql69q

For now, I will readjust the failing test cases to use tmp.tmpdir instead and use path.join to make these test cases work.

See #141

@silkentrance
Copy link
Collaborator Author

See also #156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants