Skip to content

Conversation

@curbengh
Copy link
Contributor

@curbengh curbengh commented Aug 16, 2020

ref: hexojs/hexo#4479

this enhances path.join() to convert slash, but this joinPath() doesn't resolve '../' yet.

Comment on lines +352 to +353
/* If the joined path string is a zero-length string, then '.' will be returned,
representing the current working directory. */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar behaviour as path.join()

@coveralls
Copy link

coveralls commented Aug 16, 2020

Coverage Status

Coverage increased (+0.05%) to 97.476% when pulling c1295d6 on curbengh:join-path into bf42b66 on hexojs:master.

@jiangtj
Copy link
Member

jiangtj commented Aug 17, 2020

but resolving '../' is not supported

path.resolve() is work?

@curbengh
Copy link
Contributor Author

curbengh commented Aug 17, 2020

path.resolve() is work?

I mean this joinPath() doesn't support resolving path yet, perhaps in future if there is a demand, currently it's out of scope.

for now, need to joinPath(resolve(from, to), pathtwo).


fixed unit test, ready for review.

@curbengh curbengh requested a review from a team August 17, 2020 08:32
@SukkaW
Copy link
Member

SukkaW commented Sep 7, 2020

@curbengh

Recently I have found a package on npm: https://npm.im/url-join

Have a look?

@stevenjoezhang
Copy link
Member

See also hexojs/hexo#5457
I believe there are many more pieces of code that could utilize this joinPath function.

@uiolee
Copy link
Member

uiolee commented Apr 14, 2024

I think it should be implemented based on path or any other module instead of string concatenation.

@stevenjoezhang
Copy link
Member

Yes, maybe path.posix.join will work for such cases.
Moreover, some other languages have more comprehensive libraries for path manipulation, rather than just simple string operations. I particularly like Rust's PathBuf, which is very elegant. I'm not sure if Node.js has a similar implementation.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants