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

URI-escape image filenames #10268

Merged
merged 23 commits into from Oct 13, 2022
Merged

URI-escape image filenames #10268

merged 23 commits into from Oct 13, 2022

Conversation

eric-wieser
Copy link
Contributor

@eric-wieser eric-wieser commented Mar 16, 2022

Without this change, local images with # in their name result in incorrect URLs in HTML builds.

There is already a similar call to urllib.parse.quote for file downloads, added in #9670, suggesting this is a sensible approach.

This also adds a hack to make things work for # specifically in LaTeX, but more work is likely needed for other weird character there.

Feature or Bugfix

  • Bugfix

Without this change, local images with `#` in their name result in incorrect URLs

There is already a similar call to `urllib.parse.quote` for file downloads, suggesting this is a sensible approach.
Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Could you add a testcase for this, please? Then I'll merge this soon.

@tk0miya tk0miya added this to the 4.5.0 milestone Mar 16, 2022
@eric-wieser
Copy link
Contributor Author

#9670 didn't have a test-case, so I was hoping I could get away without one here too...

I might have time for a test-case in a week or so, we'll see.

@tk0miya
Copy link
Member

tk0miya commented Mar 20, 2022

It seems #9670 contains a test-case :-)

@tk0miya
Copy link
Member

tk0miya commented Mar 26, 2022

Hmm... epubcheck and LaTeX build are failed when we use # for the filename. So escaping is not a good way to handle such files. We need to rename them on copying to the generated document.

@tk0miya tk0miya modified the milestones: 4.5.0, 5.0.0 Mar 26, 2022
@eric-wieser
Copy link
Contributor Author

It sounds like the latex code needs escaping too

@eric-wieser
Copy link
Contributor Author

Thanks for fixing the broken tests!

@eric-wieser
Copy link
Contributor Author

Is there any way to see the latex error message from CI?

@eric-wieser
Copy link
Contributor Author

I think the CI failure is actually a bug in epubcheck

@tk0miya tk0miya modified the milestones: 5.0.0, 5.x May 22, 2022
@tk0miya tk0miya changed the base branch from 4.x to 5.x May 22, 2022 12:54
@AA-Turner AA-Turner removed this from the 5.x milestone Oct 4, 2022
@AA-Turner AA-Turner added this to the 6.x milestone Oct 4, 2022
@AA-Turner
Copy link
Member

@eric-wieser it looks like the test failures are pretty simple to solve--are you up for having a look, it would be nice to get this in to 5.3.

A

@AA-Turner AA-Turner modified the milestones: 6.x, 5.3.0 Oct 13, 2022
@AA-Turner AA-Turner changed the title Escape image filenames URI-escape image filenames Oct 13, 2022
CHANGES Outdated Show resolved Hide resolved
@AA-Turner AA-Turner merged commit fa6d425 into sphinx-doc:5.x Oct 13, 2022
@AA-Turner
Copy link
Member

Thanks @eric-wieser!

A

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants