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.

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

fix: Improve cleanPath platform compatability #249

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

fix: Improve cleanPath platform compatability #249

Changes from all commits
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
5 changes: 3 additions & 2 deletions 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;
function cleanPath(filepath) {
var homeExpanded = (filepath.indexOf('~') === 0) ? path.join(os.homedir(), filepath.substr(1)) : filepath;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is renaming path to filepath a mistake because it's unrelated to the topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, okay, I thought it was about the rewrite of process.env.HOME only, will take a deeper look at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @hyperupcall,

I rechecked the change, and it looks like the naming change is helpful but out of the platform compatibility scope, so I'll respectfully suggest two solutions:

  1. Revert the naming change for now, and once this PR is merged, we can have another pull request to handle it.
  2. Keep them in this pull request, but rewrite the subject and description to properly and fully cover the naming issue.

It'll be great to ensure the commit history is clean and clear, not just to help debug and understand the code but also to make sure each change is atomic. So, I'll also encourage you to open an issue for the second change and revise its commit message to mention your thoughts. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a friendly example of how the second commit message might look like:

Rename path parameter to avoid shadowing path module

The path parameter in the cleanPath function was shadowing
the path module imported at the top of the file. This caused
issues when calling path.join(), as it was mistakenly treated
as a string method. Renaming the parameter to filepath resolves
this issue and improves code clarity.

Or a shorter version could be:

Rename path to avoid conflict with path module

Renaming the path parameter to filepath prevents shadowing
the imported path module and fixes related issues.

I hope this could be helpful 😄


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