go-shiori / obelisk

Go package and CLI tool for saving web page as single HTML file
MIT License
249 stars 18 forks source link

Minor enhancements for redirected links #11

Closed waybackarchiver closed 2 years ago

waybackarchiver commented 2 years ago

Closes #9

waybackarchiver commented 2 years ago

Will this work if there are multiple reditects?

Multiple redirects do not work, my first sense is that most scenarios where redirects occur are usually only once, but now that you mention it, I do feel some need to deal with this scenarios.

fmartingr commented 2 years ago

Multiple redirects do not work, my first sense is that most scenarios where redirects occur are usually only once, but now that you mention it, I do feel some need to deal with this scenarios.

Yeah, most redirects should be A -> B, not sure if we should consider covering more than that at the moment. Mostly because we need to handle how many redirect are we going to follow (in case of a broken link with infinite redirects or something like that). Or maybe the head method already follows all, never thought of that.

Anyway, if this is not a concern at the moment, it can be left as an issue until it is.

waybackarchiver commented 2 years ago

It should now be able to handle multiple redirects at https://github.com/go-shiori/obelisk/pull/11/commits/f77c37e4770574f5a458185e0fad5dc1eb67c8a3

fmartingr commented 2 years ago

It should now be able to handle multiple redirects at f77c37e

Awesome work! :D

waybackarchiver commented 2 years ago

Reviewed again the documentation about http.Head, in fact head requests have been implemented to follows multiple redirects automatically, perhaps we are redundant here?

% go doc http.Head
package http // import "net/http"

func Head(url string) (resp *Response, err error)
    Head issues a HEAD to the specified URL. If the response is one of the
    following redirect codes, Head follows the redirect, up to a maximum of 10
    redirects:

        301 (Moved Permanently)
        302 (Found)
        303 (See Other)
        307 (Temporary Redirect)
        308 (Permanent Redirect)

Edit: I'm not sure if there are any other scenarios where we'll need to use follow redirects again.

fmartingr commented 2 years ago

Yeah. It seems that it does exactly what you implemented. Sorry for not checking that before asking. 😥

waybackarchiver commented 2 years ago

Yeah. It seems that it does exactly what you implemented. Sorry for not checking that before asking.

It's all right. I'll easily revert both commits https://github.com/go-shiori/obelisk/pull/11/commits/309437c0905d431778069378bf5a447845abf072 and https://github.com/go-shiori/obelisk/pull/11/commits/309437c0905d431778069378bf5a447845abf072.

waybackarchiver commented 2 years ago

We'll merge this improvement now and adjust it later if any other issues arise.