-
Notifications
You must be signed in to change notification settings - Fork 330
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
epub: fix fatal errors while parsing EPUB files #1854
base: main
Are you sure you want to change the base?
Conversation
After generating the EPUB file for the Elixir docs with this version, and reviewing the result with `epubcheck`, I got the following summary: ```console $ epubcheck doc/elixir/Elixir.epub --json elixir_docs.json (base) Check finished with errors Messages: 0 fatals / 141 errors / 0 warnings / 0 infos ``` If you compare the previous result with what we had on #1851 ``` Messages: 9 fatals / 425 errors / 0 warnings / 0 infos ``` you can see that now we don't have messages with `fatal` severity and we have reduced considerably the number of errors =) I manually checked the generated EPUB on Apple Books and the previous truncated sections are solved, I don't see the banner _Below is a rendering of the page up to the first error_ and also the links to anchor different anchor seems to work. Fixes: #1851
|> String.replace(~r{id="&+/\d+[^"]*}, &String.replace(&1, "&", "&")) | ||
|> String.replace(~r{href="[^#"]*#&+/\d+[^"]*}, &String.replace(&1, "&", "&")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I frowned a little with these nested String.replace
. So, please let me now if you have any advice on how to improve this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojtekmach I though we had already escaped those when generating the links. Maybe this is something (or an option) we can pass when autolinking? The id we can fix by escaping in the document itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wojtekmach @josevalim I know it has been a while, but is there any action that's expected on my end? How do we move on/resume this discussion?
|
||
The following text includes a reference to an anchor that causes problems in EPUB documents. | ||
|
||
To remove this anti-pattern, we can replace `&&/2`, `||/2`, and `!/1` by `and/2`, `or/2`, and `not/1` respectively. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this line to demonstrate that we're transforming the links to problematic anchors in EPUB files.
here is the output for the errors using epubcheck |
After generating the EPUB file for the Elixir docs with this version, and reviewing the result with
epubcheck
, I got the following summary:If you compare the previous result with what we had on #1851
you can see that now we don't have messages with
fatal
severity and we have reduced considerably the number oferrors
=)I manually checked the generated EPUB on Apple Books and the previous truncated sections are fixed, I don't see the banner Below is a rendering of the page up to the first error and also the links to different anchors seems to work.
Fixes: #1851