proper encoding "+" in package download url #2288

Merged
merged 2 commits into from Apr 26, 2017

Conversation

Projects
None yet
3 participants
@bigbearzhu

bigbearzhu commented Apr 26, 2017

Should fix #2261 properly. Encode "+" only in base url not query string. And encode when the download link is just fetched.
See https://dotnetfiddle.net/7RwLPU for what the encodeURL does.
Also there is another bug in SemVer.Parse that parse the prerelease incorrectly when metadata is used. e.g. 2.0.54674-master+d277eaf would have master+ as prerelease, but it should be master.

@bigbearzhu bigbearzhu changed the title from proper fix for bug 2261 to proper fix for bug #2261 Apr 26, 2017

@bigbearzhu bigbearzhu changed the title from proper fix for bug #2261 to proper encoding "+" in package download url Apr 26, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Apr 26, 2017

Member

Its probably fine as it is but I'd really like to see unit tests for this. Do you feel like you could add some? Tbh we should have added them right away...

Member

matthid commented Apr 26, 2017

Its probably fine as it is but I'd really like to see unit tests for this. Do you feel like you could add some? Tbh we should have added them right away...

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 26, 2017

Member

since this is better than what we have and actually fixes a bug I'm willing to merge it now. But please send a nother PR with a unit test. Thx

Member

forki commented Apr 26, 2017

since this is better than what we have and actually fixes a bug I'm willing to merge it now. But please send a nother PR with a unit test. Thx

@forki forki merged commit ecf5ccb into fsprojects:master Apr 26, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bigbearzhu

This comment has been minimized.

Show comment
Hide comment
@bigbearzhu

bigbearzhu Apr 26, 2017

Thanks for quick merge. I tried to add some test before sending the pull request, didn't find existing tests for either NuGetV2.fs or SemVer.fs. The SemVerTests.cs seems is for the bootstrapper only. Mind pointing me to the right place/way? I only use F# occasionally, not very good at doing big refactoring jobs.

Thanks for quick merge. I tried to add some test before sending the pull request, didn't find existing tests for either NuGetV2.fs or SemVer.fs. The SemVerTests.cs seems is for the bootstrapper only. Mind pointing me to the right place/way? I only use F# occasionally, not very good at doing big refactoring jobs.

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