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

Add support for image links #92

Closed
dcastro opened this issue Oct 31, 2021 · 4 comments · Fixed by #183
Closed

Add support for image links #92

dcastro opened this issue Oct 31, 2021 · 4 comments · Fixed by #183
Assignees
Labels
feature New functionality
Milestone

Comments

@dcastro
Copy link
Member

dcastro commented Oct 31, 2021

Clarification and motivation

We should add support for image links.

![alt text](https://localhost/image.png "Title/Tooltip Text")
![alt text][image-ref]

[image-ref]: https://localhost/image.png "Title/Tooltip Text"

Acceptance criteria

xrefcheck verifies whether image links are valid.

@Martoon-00
Copy link
Member

For references that point to images, we will need special care. The current approach to verifying links (load page with HEAD method, then with GET) may turn out to be very heavyweight when it comes to resources.

@dcastro
Copy link
Member Author

dcastro commented Nov 3, 2021

For references that point to images, we will need special care. The current approach to verifying links (load page with HEAD method, then with GET) may turn out to be very heavyweight when it comes to resources.

Oh good point. We can hope that the server will send a chunked response, in which case we only need to wait for the first chunk to inspect the status code (and not look at the response's body at all).

We should probably also have a config/CLI option to disable checking images, in case the user's project happens to have a lot of links to heavy images.

@dcastro dcastro added the feature New functionality label Nov 3, 2021
@dcastro dcastro added this to the 0.3 milestone Sep 17, 2022
@dcastro
Copy link
Member Author

dcastro commented Sep 17, 2022

Thinking about this a bit more - maybe they don't need special treatment (nor a flag to disable this check)?

I mean, from our point of view, an image link like ![alt text](https://localhost/image.png) is the exact same thing as a regular link like [alt text](https://localhost/image.png), and we don't treat those as special. In fact, a user can currently use a regular link to point to a 1GB binary file.


Since we're on this topic, I also tried this:

[link](https://ftp.rnl.tecnico.ulisboa.pt/pub/ubuntu/releases/22.04.1/ubuntu-22.04.1-desktop-amd64.iso)

And xrefcheck returns almost immediately.

$ time xrefcheck
Configuration file not found, using default config for GitHub repositories

All repository links are valid.                                                                                                                     
xrefcheck  0.84s user 0.42s system 216% cpu 0.583 total

@Martoon-00
Copy link
Member

Oh really. I initially has an impression, that performing a request usually results in getting the entire response, but in fact most libraries provide a response as a stream, not the entire response at once. And req even allows discarding the response without fetching it, which we in fact currently do.

Nice observation.


Maybe we don't even need to try the request with HEAD method first then, that could be an oversight from my side too. The only purpose of this extra action was to avoid reading potentially large response bodies, but this also comes with obvious drawbacks.

@Sereja313 Sereja313 self-assigned this Sep 28, 2022
Sereja313 added a commit that referenced this issue Sep 29, 2022
Problem: We should add support for image links.

Solution: Extract image links as regular links.
Sereja313 added a commit that referenced this issue Sep 29, 2022
Problem: We should add support for image links.

Solution: Extract image links as regular links.
Sereja313 added a commit that referenced this issue Oct 25, 2022
Problem: We should add support for image links.

Solution: Extract image links as regular links.
Sereja313 added a commit that referenced this issue Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants