-
Notifications
You must be signed in to change notification settings - Fork 167
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
Attaching relative or absolute files #12
Comments
Agree, will be good to fix this. |
My pull request should fix the issue - I've been using it for my projects for the last couple of days and it works for the scenarios I'm using it in. It would be good to have some tests for that part of the code, though. Note: I'm aware that my pull request has failed to build on Travis CI - that's because the whole library currently isn't building. The solution looks to be changing the Travis config to require Node 0.10 rather than 0.10.15, as the latter isn't installed. I'll submit a fix tonight. |
Looked over the pull request, seems to be fine, merged and fix is included in v0.2.7! Updated travis.yml as well 👍 |
Woohoo! Thank you! 👍 |
Hiya,
I'm trying to attach a file to a request and running into some problems with building a file path that unirest will handle in a sensible manner.
On Linux I have either:
data/file.txt
/opt/myapp/data/file.txt
(where the cwd is
/opt/myapp
)On Windows my paths look like:
data\file.txt
C:\myapp\data\file.txt
(where the cwd is
C:\myapp
)Unirest expects file paths in one of three formats:
http://
orhttps://
for remote files (which I'm not trying to do)://
for absolute paths - which doesn't match up to what either Linux or Windows absolute paths would look like__dirname
, which is normally./node_modules/unirest
, a highly unlikely place to want to upload files from.I've summed it up in the following Gist:
https://gist.github.com/cosmicflame/dd4bf009f6f7ababaa54
(Apologies if it's somewhat rambling... it's quite late where I am)
The two solutions I've found for forcing absolute paths are:
C://myapp/data/file.txt
- that is, forcing a://
in there, even though that's not platform-native. Yuck.path.relative('./node_modules/unirest', myAbsolutePath)
- that is, turning my absolute path into a relative path that's relative to where unirest is installed. Double yuck.The code that's doing the file loading is this:
I think we should ditch the
://
check (unless I'm missing something here...) and instead usepath.resolve()
(http://nodejs.org/api/path.html) for both absolute and relative paths. Something like this:This will turn both absolute and relative paths into absolute paths, which will then be read from disk correctly, on both Windows and Linux.
I'll happily make the change and send you a pull request if you'd be willing to accept it.
Thanks,
B~
The text was updated successfully, but these errors were encountered: