Skip to content

fix: Improve cleanPath platform compatability #249

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.

Sign up for GitHub

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

Open
hyperupcall wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: Improve cleanPath platform compatability #249

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76e18ce
fix: Improve `cleanPath` platform compatability
hyperupcall Oct 12, 2023
738f1cc
fix: Use of `path`
hyperupcall Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion doctoc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

var path = require('path')
, fs = require('fs')
, os = require('os')
, 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;
Copy link
Collaborator

@PeterDaveHello PeterDaveHello Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@hyperupcall hyperupcall Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Escape all spaces
return homeExpanded.replace(/\s/g, '\\ ');
Expand Down