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

This patch allows FlagRemoveDotSegments to work with relative links -- #5

Merged
merged 1 commit into from
Jan 22, 2015

Conversation

pchristopher1275
Copy link
Contributor

i.e. URL's that have the Host field set to the empty string.
Just to be clear, relative links will be stored in URL's,
typically, if the user expects to later call URL.ResolveReference
to create a full URL. That seems to be a relatively common use-case.

i.e. URL's that have the Host field set to the empty string.
Just to be clear, relative links will be stored in URL's,
typically, if the user expects to later call URL.ResolveReference
to create a full URL. That seems to be a relatively common use-case.
@pchristopher1275
Copy link
Contributor Author

Mentioning @PuerkitoBio so he can see this fork.

mna added a commit that referenced this pull request Jan 22, 2015
This patch allows FlagRemoveDotSegments to work with relative links --
@mna mna merged commit 9a3828e into PuerkitoBio:master Jan 22, 2015
@mna
Copy link
Member

mna commented Jan 22, 2015

Thanks for the PR!

anthonyfok added a commit to gohugoio/hugo that referenced this pull request Jan 23, 2015
This reverts commit 71fe85d.

See PuerkitoBio/purell#5
for the cause of the mysterious `go test -v ./...` failure.
@anthonyfok
Copy link

Hi! This PR has caused some new errors to show up in the Hugo Travis CI test running go test -v ./..., e.g.:

I am not saying that there is anything wrong with this PR (I haven't looked at it yet, and I have to go outside in a few minutes), but just a note that some behaviour has changed that needs investigation. :-)

@mna
Copy link
Member

mna commented Jan 23, 2015

Thanks, will revert for not until I have time to investigate further.

On Thu, Jan 22, 2015, 16:53 Anthony Fok notifications@github.com wrote:

Hi! This PR has caused some new errors to show up in the [Hugo](
https://github.com/spf13/hugo` Travis CI test running go test -v ./...,
e.g.:

https://travis-ci.org/spf13/hugo/jobs/47995348

I am not saying that there is anything wrong with this PR (I haven't
looked at it yet, and I have to go outside in a few minutes), but just a
note that some behaviour has changed that needs investigation. :-)


Reply to this email directly or view it on GitHub
#5 (comment).

@anthonyfok
Copy link

Hi @PuerkitoBio,

Wow, I didn't expect such a quick response! Actually, perhaps this "patch to allows FlagRemoveDotSegments to work with relative links" is good, and perhaps it is Hugo that needs to modified for the potentially improved behaviour in purell. :-) Anyhow, I think I will leave this to people more capable than me like you, @bjornerik, @tatsushid and @owenwaller to look into. :-)

Thanks again!

@mna
Copy link
Member

mna commented Jan 23, 2015

Oh sure , that revert is absolutely not a blame of the pr, it's just that I
can't take care of it right now so until then I'd rather not break people's
stuff until we understand what causes the change of behaviour.
On Thu, Jan 22, 2015 at 22:04 Anthony Fok notifications@github.com wrote:

Hi @PuerkitoBio https://github.com/PuerkitoBio,

Wow, I didn't expect such a quick response! Actually, perhaps this "patch
to allows FlagRemoveDotSegments to work with relative links" is good, and
perhaps it is Hugo that needs to modified for the potentially improved
behaviour in purell. :-) Anyhow, I think I will leave this to people more
capable than me like you, @bjornerik https://github.com/bjornerik,
@tatsushid https://github.com/tatsushid and @owenwaller
https://github.com/owenwaller to look into. :-)

Thanks again!


Reply to this email directly or view it on GitHub
#5 (comment).

@pchristopher1275
Copy link
Contributor Author

Thanks Martin! Looks like this is out of my hands right now. But I'll keep
my eyes peeled if there is something on this patch that I'll need to fix.
Thanks again.

-pete

On Thu, Jan 22, 2015 at 11:30 PM, Martin Angers notifications@github.com
wrote:

Oh sure , that revert is absolutely not a blame of the pr, it's just that
I
can't take care of it right now so until then I'd rather not break
people's
stuff until we understand what causes the change of behaviour.
On Thu, Jan 22, 2015 at 22:04 Anthony Fok notifications@github.com
wrote:

Hi @PuerkitoBio https://github.com/PuerkitoBio,

Wow, I didn't expect such a quick response! Actually, perhaps this
"patch
to allows FlagRemoveDotSegments to work with relative links" is good,
and
perhaps it is Hugo that needs to modified for the potentially improved
behaviour in purell. :-) Anyhow, I think I will leave this to people
more
capable than me like you, @bjornerik https://github.com/bjornerik,
@tatsushid https://github.com/tatsushid and @owenwaller
https://github.com/owenwaller to look into. :-)

Thanks again!


Reply to this email directly or view it on GitHub
#5 (comment).


Reply to this email directly or view it on GitHub
#5 (comment).

@dankinder
Copy link

Hi guys, any word on this? Since this was reverted should the pull request be reissued?

@mna
Copy link
Member

mna commented Feb 3, 2015

Sorry, I didn't have much time to go back to this yet. No need to resubmit, I can revert the revert if required.

@mna
Copy link
Member

mna commented Feb 5, 2015

The PR is ready in the fix-relative-paths branch, I opened an issue in the hugo repository to give them a chance to update their code first so that we don't break things. Hopefully it's a simple fix on their side and we can synchronize a merge to master soon. I do believe the PR is correct and does fix a buggy behavior of purell with relative paths.

tychoish pushed a commit to tychoish/hugo that referenced this pull request Aug 13, 2017
This reverts commit 71fe85d.

See PuerkitoBio/purell#5
for the cause of the mysterious `go test -v ./...` failure.
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.

4 participants