-
Notifications
You must be signed in to change notification settings - Fork 239
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
Improved URL Parsing #111
Improved URL Parsing #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Brendan, thanks for the pull request.
Changes are looking good. I've a few requests though:
- Could you add tests please? You can find sample tests at
e2e/run_test.go
. - We vendor dependencies. Could you run
go mod tidy && go mod vendor
and commit the changes?
Thanks
Follow up for improved URL parsing
Hey @igungor, thanks for the review. Are there any steps I need to take before running the tests locally? I'm seeing a bunch of errors similar to this:
|
Some of the tests are failing due to the new New
That'd be very helpful if you could provide a concrete use-case to demonstrate the problem. We can debug the issue as well. |
Ping @brendan-matroid |
Hey @igungor, sorry for the delay.
At If |
I'm OK with this change. I think it is necessary.
I understand the problem now, thanks for the detailed answer. The problem with this change is that Could you try and check if it's working for your use case? If so, and if you're still interested, could you rebase your branch please, so that we can progress. |
Fix tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR again. Mostly LGTM with minor comments.
will re-open to trigger circle-ci |
let's try again now. |
shellquote
to split lines incommands.txt
files so filenames with spaces can be usedprefix
es when initializingObjectURL
s. These may be a part of valid S3 prefixes.