Skip to content
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

Path should format when 'base' missing and 'name' and 'ext' exist #2305

Closed
reggi opened this issue Aug 5, 2015 · 4 comments
Closed

Path should format when 'base' missing and 'name' and 'ext' exist #2305

reggi opened this issue Aug 5, 2015 · 4 comments
Labels
doc Issues and PRs related to the documentations. feature request Issues that request new features to be added to Node.js. path Issues and PRs related to the path subsystem.

Comments

@reggi
Copy link
Contributor

reggi commented Aug 5, 2015

This test shows that name and ext are not taken into account when using path.format without a base.

The common usage this is preventing is this:

var src = path.parse(srcpath)
src.base = null
if(src.ext === '') src.ext = '.js'
src = path.format(src)

However this doesn't do anything.

var path = require('path')
var assert = require('assert')

// pulled from https://nodejs.org/api/path.html#path_path_format_pathobject
/* global describe, it */

describe('node path', function () {

  it('should work as documented', function () {
    assert.equal(path.format({
      root: '/',
      dir: '/home/user/dir',
      base: 'file.txt',
      ext: '.txt',
      name: 'file'
    }), '/home/user/dir/file.txt')
  })

  it('should not work if missing base', function () {
    assert.notEqual(path.format({
      root: '/',
      dir: '/home/user/dir',
      ext: '.txt',
      name: 'file'
    }), '/home/user/dir/file.txt')
  })

  it('should show ext and name are irrelevant', function () {
    assert.equal(path.format({
      root: '/',
      dir: '/home/user/dir',
      ext: '.txt',
      name: 'file'
    }), path.format({
      root: '/',
      dir: '/home/user/dir'
    }))
  })

})

https://github.com/nodejs/io.js/blob/master/lib/path.js#L584

I can write a pull request, if anyone else thinks this is a good idea.

@mscdex mscdex added the path Issues and PRs related to the path subsystem. label Aug 5, 2015
@sam-github
Copy link
Contributor

Your example code above is really confusing, its not clear whether you are posting a unit test that should pass, but does not pass right now, or that passes right now, but that should not.

For future readers, the problem is that the unit test does pass, and that's lame:

> path.format({ root: '/', dir: '/home/user/dir', base: 'file.txt', ext: '.js', name: 'other' })
'/home/user/dir/file.txt'

This is somewhat as I would expect, because it parallels how url.format() works when it uses host if it exists, and only uses the component hostname and port properties if host does not exist.

> path.format({ root: '/', dir: '/home/user/dir', ext: '.js', name: 'other' })
'/home/user/dir/'

This is bizarre, I would expect name and .ext to be concatenated into the file base, as would @reggi I assume.

Also, note that path.format() is undocumented: https://iojs.org/api/path.html#path_path_format_pathobject

One example of how it behaves with a single set of inputs is not documentation :-(. Its also a particularly bad example, because it implies that the root, name, and ext properties are input... they are not, they are completely ignored.

@reggi I'd +1 a fix for this, particularly one that actually added documentation in detail, in a form paralleling url.format(), that is sufficiently detailed to be able to predict the output from any set of inputs, without experimenting at the repl. For example:

> path.format({ root: '/root', base: 'helo.you', ext: '.js', name: 'other' })
'helo.you'

What happened to root? Is root even used? Seems not...

@sam-github sam-github added feature request Issues that request new features to be added to Node.js. doc Issues and PRs related to the documentations. path Issues and PRs related to the path subsystem. and removed path Issues and PRs related to the path subsystem. labels Aug 5, 2015
@reggi
Copy link
Contributor Author

reggi commented Aug 6, 2015

@sam-github Thanks for looking into the topic and sorry for the ambiguous test, should have mentioned that it passes. I completely agree on paralleling url.format().

@Trott
Copy link
Member

Trott commented Mar 12, 2016

Except for the documentation issues cited by @sam-github, this all appears to be fixed. If no one beats me to it (and please, feel free), I'll try to get around to documenting this soon so we can close this out.

Trott added a commit to Trott/io.js that referenced this issue Mar 13, 2016
@Trott Trott closed this as completed in a97dfa0 Mar 16, 2016
@Trott
Copy link
Member

Trott commented Mar 16, 2016

Documentation landed in a97dfa0.

MylesBorins pushed a commit that referenced this issue Mar 17, 2016
PR-URL: #5688
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: #2305
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
PR-URL: #5688
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: #2305
MylesBorins pushed a commit that referenced this issue Mar 21, 2016
PR-URL: #5688
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: #2305
Fishrock123 pushed a commit that referenced this issue Mar 22, 2016
PR-URL: #5688
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: #2305
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. feature request Issues that request new features to be added to Node.js. path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants