fix: Improve cleanPath platform compatability
#249
Conversation
doctoc.js
Outdated
| , minimist = require('minimist') | ||
| , file = require('./lib/file') | ||
| , transform = require('./lib/transform') | ||
| , files; | ||
|
|
||
| function cleanPath(path) { | ||
| var homeExpanded = (path.indexOf('~') === 0) ? process.env.HOME + path.substr(1) : path; | ||
| var homeExpanded = (path.indexOf('~') === 0) ? path.join(os.homedir() + path.substr(1)) : path; |
There was a problem hiding this comment.
While using os.homedir() indeed improves cross-platform compatibility, I noticed that the introduction of path.join() deviates slightly from the existing style. I'm unsure if this stylistic change was intended for this PR.
Additionally, if path.join() is used, it should accept multiple arguments (e.g., path.join(os.homedir(), path.substr(1))) rather than a single concatenated string.
Let me know your thoughts on this!
There was a problem hiding this comment.
Agreed! Embarrassingly, when I first tested these changes I must have forgotten to quote:
$ ./doctoc.js ~/Documents/...$ ./doctoc.js '~/Documents/...'I re-ran and it works as expected. I also fixed up the code to allow for using path.join by renaming the parameter in the function. path.join is also more cross platform.
| function cleanPath(path) { | ||
| var homeExpanded = (path.indexOf('~') === 0) ? process.env.HOME + path.substr(1) : path; | ||
| function cleanPath(filepath) { | ||
| var homeExpanded = (filepath.indexOf('~') === 0) ? path.join(os.homedir(), filepath.substr(1)) : filepath; |
There was a problem hiding this comment.
Is renaming path to filepath a mistake because it's unrelated to the topic?
There was a problem hiding this comment.
Nope it's intentional; I renamed path to filepath because the path parameter in the cleanPath function was shadowing the path variable defined at the top of the file (var path = require('path')). And this caused a bug, since path.join is a function exported from the path module - String.prototype.join() does not exist. A mistake in my testing prevented me from catching this earlier
As I understand both files and folders can be passed to the cleanPath function, maybe you prefer a different rename?
There was a problem hiding this comment.
Oh, okay, I thought it was about the rewrite of process.env.HOME only, will take a deeper look at it.
Uses
os.homedir()since that works better on non-POSIXy platforms.I know this code uses old conventions and methods, so if NodeJS version compatability is a concern (even though they have been unsupported for decades), these are when these functions were added to NodeJS:
v2.3.0v0.1.16