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

Fix absolute and relative paths #5

Closed
wants to merge 1 commit into from
Closed

Conversation

rumkin
Copy link

@rumkin rumkin commented Aug 3, 2015

fs.resolve allow to specify relative paths appending process.cwd() but skip absolute paths.

fs.resolve allow to specify relative paths appending process.cwd() but skip absolute paths.
@flesler
Copy link
Owner

flesler commented Aug 3, 2015

Hi, what's the purpose of this change? did you have any problem?

@rumkin
Copy link
Author

rumkin commented Aug 4, 2015

Yep. I was trying to read configs which are stored in directory outside of application and CWD, but could not do that with absolute nor relative paths.

@oger000
Copy link

oger000 commented Jun 29, 2016

Thank you flesler for for that module - and thank you rumkin for making it useable for my needs.

@flesler flesler closed this in df27ca9 Jun 29, 2016
@flesler
Copy link
Owner

flesler commented Jun 29, 2016

@rumkin @oger000 I made a commit that includes the line you suggested plus tests, a version bump and more.

Can you guys try this version and confirm it now works? I won't publish it to NPM until it's confirmed.
Just in case I made a minor version bump because I'm not 100% sure it won't break stuff.

I noted that, on Windows+GitBash, using something like __dirname will break. I don't think this was supported before so this is still a positive change regardless.

Thanks

flesler added a commit that referenced this pull request Jun 29, 2016
@flesler
Copy link
Owner

flesler commented Jun 29, 2016

UPDATE: Tried using path.resolve() instead of path.join(), it solved the issue with Windows and also resolves relative paths without the need to prepend process.cwd() explicitely.

Can you please re-test? I added 2 tests and they all pass.
Thanks

@flesler
Copy link
Owner

flesler commented Jul 4, 2016

Releasing it then

@rumkin
Copy link
Author

rumkin commented Jul 5, 2016

I can't test it on windows. Tested on ubuntu 14.04 and MacOS. All tests are passed.

@flesler
Copy link
Owner

flesler commented Jul 5, 2016

Does your case now work with these changes?

@rumkin
Copy link
Author

rumkin commented Jul 6, 2016

Yes, it works fine. Thanks!

@oger000
Copy link

oger000 commented Jul 7, 2016

Works for me.

@flesler
Copy link
Owner

flesler commented Jul 7, 2016

Great, I published 1.3.0. Just in case I bumped the minor version

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.

None yet

3 participants