EPUB support by milmazz · Pull Request #319 · elixir-lang/ex_doc

milmazz · 2015-08-28T03:40:07Z

This is a work in progress, the idea is to know your opinion about the current progress.

Things to do:

Improve styles
Fix known issues

Tasks already done:

EPUB document display correctly, at least on iBooks, Calibre and Android Aldiko
Basic templates (Table of Contents, Cover, Module documentation, stylesheets, assets, etc)
Support for EPUB 2 and EPUB 3
Reuse Autolink from the HTML formatter.
Fix all issues reported by Calibre (Tools ▶️ Check Book)
Add tests
Support for logo option
Support for extras option

Some of the known bugs are the following:

Errors reported by epubcheck
When ExDoc is executed from the command line (../ExDoc/bin/ex_doc ... -m Kernel -f epub) I'm getting an UndefinedFunctionError (the problem is related with the new UUID dependency), but everything runs OK via MIX_ENV=docs mix docs -f epub, any advice? Fix: e4e5c65
According to the epubcheck tool the mimetype file contains a wrong type, but I don't know yet the reason for this (Am I using the :zip.create function correctly?). This file needs to comply with the following rules Fix: 0e8dab4
~~The other annoying error reported by epubcheck is related with the ids (anchor) generated by Autolink.~~ Fix: e680b00

ERROR: Ecto-v0.16.0.epub/OEBPS/modules/Ecto.Adapter.html(163,44): value of attribute "id" is invalid; must be an XML name without colons
WARNING: Ecto-v0.16.0.epub/OEBPS/modules/Ecto.Adapter.html(173,49): use of non-registered URI scheme type in href: Ecto.Type.html#t:t/0

One way to solve this is to post-process the results from the templates, something like this:

  defp generate_module_page(output, config, node) do
    content = Templates.module_page(config, node) |> clean_ids_hrefs()
    File.write("#{output}/OEBPS/modules/#{node.id}.html", content)
  end

  defp clean_ids_hrefs(content) do
    id_pattern = ~r/[^A-Za-z0-9_.-]/

    result = Regex.replace(~r/href=\s*"(?!(http|https|ftp|mailto):\/\/)([^#]+)?#([^"]+)"/i, content, fn _, _, page, anchor -> 
"href=\"#{page}#" <> String.replace(anchor, id_pattern, "") <> "\"" end)
    Regex.replace(~r/id="([^\"]+)"/, result, fn _, id -> "id=\"" <> String.replace(id, id_pattern, "") <> "\"" end)
  end

But this to me seems like an ugly hack and maybe with this hack we can create a duplicate ID (I need to verify this, but each duplicate ID will be reported by epubcheck as an error), but I want to reuse ExDoc.Formatter.HTML.Autolink as much as possible. Any advice?

Example for Ecto:

$ export MIX_ENV=docs
$ mix deps.update ex_doc
$ mix deps.compile ex_doc
$ mix docs -f epub
$ cd doc
$ ls
Ecto-v1.0.0.epub
# How to use epubcheck
$ java -jar ../epubcheck-3.0.1/epubcheck-3.0.1.jar Ecto-v1.0.0.epub

Closes #305

milmazz · 2015-08-28T15:16:50Z

@josevalim ping

milmazz · 2015-09-23T03:39:49Z

@josevalim Ping, any review is greatly appreciated.

At this stage, I think that this new feature is in beta and if you think it's right moment we can merge into master.

Thanks in advance 👌

josevalim · 2015-09-23T10:15:04Z

Thank you @milmazz! I need to prepare for my keynote and release Elixir v1.1. So let's push ExDoc 0.10 first and then I will focus on this with you. ❤️

milmazz · 2015-09-23T14:07:57Z

No problem @josevalim, in the meantime I'll try to improve the CSS styles,
add more documentation, tests and fix other things :)

I hope that everything goes well on the ElixirConf ❤️

henrik · 2015-09-27T10:01:26Z

Would it make any sense to extract some of the epub generation to a general-purpose epub lib?

josevalim · 2015-09-27T10:06:56Z

We haven't even landed epub support yet. Once we do and once it works, we can possibly start a discussion about extracting it. :D

henrik · 2015-09-27T10:41:06Z

That's completely fair. I was mostly thinking that this could make this PR smaller and also give @milmazz something to do until this is back in focus :D @josevalim, best of luck with the keynote! Wish I could be there :)

milmazz · 2015-09-29T00:24:10Z

@henrik In the meantime you can review the code if you want, any feedback is more than welcome :)

I'll try to update this PR as soon as possible.

dukex · 2015-10-02T11:42:23Z

lib/ex_doc/formatter/epub.ex

Here is possible, I'm new in elixir, only return :zip.create... eg:

defp generate_epub(output, config) do output = Path.expand(output) target_path = "#{output}/#{config.project}-v#{config.version}.epub" |> String.to_char_list :zip.create(target_path, files_to_add(output), compress: ['.css', '.html', '.ncx', '.opf', '.jpg', '.png', '.xml']) end

On line 42 you are doing the check if is :ok

That's a common Elixir practice. Somtimes it is preferable to match on the result when we call it so we get the proper cause on errors: http://blog.plataformatec.com.br/2014/09/writing-assertive-code-with-elixir/

but isn't the last line "{:ok, zip_path}" unnecessary? since that's what the previous line returns

Yes, the last line can be deleted, I'll fix that!

milmazz · 2016-09-11T22:40:09Z

@josevalim This weekend I finally resume working on this feature, I would like your review about this. If you approve this feature in master, then, we definitely will need some help in the design area, but I think the current behavior is a good start, wdyt?

milmazz · 2016-09-13T17:07:19Z

@josevalim ping?

josevalim · 2016-09-14T09:28:07Z

lib/ex_doc/formatter/epub/templates.ex

+  defp extra_title(path), do: path |> String.upcase |> Path.basename(".MD")
+
+  templates = [
+    content_template: {[:config, :nodes, :uuid, :datetime], @content_template_doc},


Since you want to attach docs, I would rather call EEx.function_from_file one by one instead of building this whole list. :)

josevalim · 2016-09-14T09:30:56Z

@milmazz this looks great to me! I have added only one tiny comment. The other thing that is missing is the documentation aspect. How to use EPUB? I would add some information to the README as well to the mix docs task.

Another point of concern is code duplication between the formatters but I haven't taken a deep look yet. What do you think? Were you able to re-use most of the code? Is there any area you would like to avoid the duplication bit?

In any case, feel free to merge it to master any time you want.

milmazz · 2016-09-22T04:09:42Z

gulpfile.js

+
+gulp.task('epub-javascript', ['buildHighlight'], function () {
+  return javascript({src: 'assets/js/epub.js', dest: distPath.epub}, function () {});
 })


@dignifiedquire Do you think this is a good way to reuse this task without affecting the current behavior?

It is fine to affect the current behaviour though since this is an internal tool.

milmazz · 2016-09-22T04:40:04Z

@josevalim I just add more improvements, this is the current status:

About your suggestions:

The other thing that is missing is the documentation aspect. How to use EPUB? I would add some information to the README as well to the mix docs task.

You're right, I'll add more information about this new feature asap.

Another point of concern is code duplication between the formatters but I haven't taken a deep look yet. What do you think? Were you able to re-use most of the code?

Yeah, I know, I tried to avoid the code duplication between both formats as much as possible. Actually, I use many functions from the ExDoc.Formatter.HTML, ExDoc.Formatter.HTML.Autolink and some functions from ExDoc.Formatter.HTML.Templates. At least the ExDoc.Formatter.EPUB.run/2 function follow almost the same workflow as ExDoc.Formatter.HTML.run/2, but, there are a lot of different templates between both formats. Although, I'll check this PR again and see if I miss something else :)

@elixir-lang/exdoc Any other suggestion is more than welcome.

josevalim · 2016-09-22T08:46:53Z

It looks beautiful. I should do another full review pass today or tomorrow and add any pending feedback.

josevalim · 2016-09-22T15:31:48Z

assets/README.md

 Using the flag `--type production` will result in minified JavaScript and CSS
 bundles.

+### `epub-build`


Would it possible to have build do both epub and html builds? This way we should still keep build but we can then use the epub-build and html-build only if I want explicitly too.

Sure, I'll change that.

josevalim · 2016-09-22T15:32:26Z

assets/README.md


 Build the JavaScript in `js` into a bundled file using [webpack].

+### `epub-javascript`


Should we normalize all names to be FORMATTER-TASK? So we would have epub-javascript and html-javascript. Otherwise I am afraid it will be confusing. What do you think?

I agree with your suggestion.

josevalim · 2016-09-22T15:41:34Z

lib/ex_doc/formatter/epub/templates/content_template.eex

+    <%= for node <- nodes do %>
+      <item id="<%= node.id %>" href="<%= node.id %>.xhtml" media-type="application/xhtml+xml" properties="scripted"/>
+    <% end %>
+    <% alias ExDoc.Formatter.HTML.Templates, as: T %>


We should probably move those aliases to the top of the template OR to the module that defines the templates.

Better to move the alias to the module that uses the templates, that way we avoid a blank line at the beginning of this XML file (epubchecker emits a warning for this).

josevalim · 2016-09-22T15:41:59Z

@milmazz i have added a couple more comments, then we are good to go!

josevalim · 2016-09-22T21:04:27Z

@milmazz I did another review and it looks amazing! ❤️

The only thing left I can think of is to add support for the --formatter flag in the mix task and document it here: https://github.com/elixir-lang/ex_doc/blob/master/lib/mix/tasks/docs.ex

milmazz · 2016-09-23T01:38:20Z

@josevalim Done! 🎉

$ mix docs --formatter epub
Compiling 1 file (.ex)
Docs successfully generated.
View them at '/Users/milmazz/Development/elixir-lang/ex_doc/doc/ex_doc-v0.13.2.epub'.
$ mix docs -f epub
Docs successfully generated.
View them at '/Users/milmazz/Development/elixir-lang/ex_doc/doc/ex_doc-v0.13.2.epub'.

dignifiedquire · 2016-09-26T13:35:38Z

@milmazz please fix the functions you created in https://github.com/elixir-lang/ex_doc/blob/master/gulpfile.js#L202 (javascript and less), they do not need to take a callback but rather return the gulp pipeline they crate, i.e. it should look like this

var javascript = function (options) {
  return gulp.src(options.src)
    .pipe(webpack(isProduction ? config.production : config.development))
    .pipe($.if(isProduction, $.uglify()))
    .pipe($.if(isProduction, $.rev()))
    .pipe($.size({title: 'js'}))
    .pipe(gulp.dest(options.dest))
}

otherwise gulp will not handle these tasks correctly as the async nature of the operations is not take into account when running the tasks and you could end up with broken builds.

milmazz · 2016-09-26T14:02:13Z

@dignifiedquire Thanks for your suggestion. I already made the change, please let me know what do you think.

dignifiedquire · 2016-09-26T14:08:25Z

@milmazz LGTM :)

milmazz force-pushed the epub branch 5 times, most recently from 50a8c11 to 3763888 Compare September 14, 2015 16:07

milmazz force-pushed the epub branch 7 times, most recently from fd3ff34 to d0c72da Compare September 22, 2015 05:45

milmazz force-pushed the epub branch 3 times, most recently from a9d91ba to a587b99 Compare September 23, 2015 03:28

milmazz force-pushed the epub branch from a587b99 to 813c049 Compare September 29, 2015 02:32

dukex reviewed Oct 2, 2015
View reviewed changes

milmazz force-pushed the epub branch from 7511fec to 5b305ca Compare October 5, 2015 11:29

milmazz force-pushed the epub branch from 5b305ca to 863599c Compare October 27, 2015 02:57

milmazz force-pushed the epub branch from 863599c to 8d5ba55 Compare February 12, 2016 02:52

milmazz force-pushed the epub branch 5 times, most recently from 1180ac6 to 473bbed Compare September 11, 2016 22:27

milmazz force-pushed the epub branch from 473bbed to e5cf350 Compare September 11, 2016 23:16

josevalim reviewed Sep 14, 2016
View reviewed changes

milmazz force-pushed the epub branch 2 times, most recently from cb57073 to 890d339 Compare September 22, 2016 04:07

milmazz commented Sep 22, 2016

View reviewed changes

josevalim reviewed Sep 22, 2016

View reviewed changes

Add EPUB formatter

a23b5f7

milmazz force-pushed the epub branch from 39a0d17 to a23b5f7 Compare September 23, 2016 01:31

milmazz merged commit da63fc0 into master Sep 23, 2016

milmazz deleted the epub branch September 23, 2016 01:43


		Build the JavaScript in `js` into a bundled file using [webpack].

		### `epub-javascript`

Conversation

milmazz commented Aug 28, 2015 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milmazz commented Aug 28, 2015

Uh oh!

milmazz commented Sep 23, 2015

Uh oh!

josevalim commented Sep 23, 2015

Uh oh!

milmazz commented Sep 23, 2015

Uh oh!

henrik commented Sep 27, 2015

Uh oh!

josevalim commented Sep 27, 2015

Uh oh!

henrik commented Sep 27, 2015

Uh oh!

milmazz commented Sep 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milmazz commented Sep 11, 2016

Uh oh!

milmazz commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Sep 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milmazz commented Sep 22, 2016

Uh oh!

josevalim commented Sep 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josevalim commented Sep 22, 2016

Uh oh!

josevalim commented Sep 22, 2016

Uh oh!

milmazz commented Sep 23, 2016

Uh oh!

dignifiedquire commented Sep 26, 2016

Uh oh!

milmazz commented Sep 26, 2016

Uh oh!

dignifiedquire commented Sep 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

milmazz commented Aug 28, 2015 •

edited

Loading