Skip to content

Conversation

@milmazz
Copy link
Member

@milmazz milmazz commented Nov 4, 2015

This PR removes the extra- prefix from the additional documentation (specified with the option extras), this was suggested by @wsmoak, you can see more details about the discussion here

@josevalim If you can, please let me know what do you think or if I need to fix something else.

As a side note, I also worked in another solution, moving all the additional documentation into the subdirectory extras, but, I like more this way.

@milmazz
Copy link
Member Author

milmazz commented Nov 4, 2015

About this statement from @wsmoak:

I'm not seeing any conflicts (files getting overwritten) in the few projects I've tried it with.

We can take some precautions about this possible issue, for example, if the filesystem is case-sensitive a collision is unlikely because all the module pages begin with uppercase and the additional documentation file names are down-cased. But this scenario is not always the case. So, we can raise an exception, something like this:

if File.regular? output do
  raise ArgumentError, "the file #{output} already exists"
else
  File.write! output, html
end

@eksperimental
Copy link
Contributor

@milmazz I think if there is a collision, not files at all should be generated, so we should check before writing anything, if a collition exists, and raise if that happens.
I will submit my comments to the mailing list where the discussion seems to be taking place.

@josevalim
Copy link
Member

I will stop being stubborn, so 👍 from me.

It would be nice to warn when there is a conflict but it is worth having in mind that we often override the previously generated docs when generating the new ones. So I am not sure if we actually should warn or raise... maybe we should just keep things exactly as in this PR.

@josevalim
Copy link
Member

Actually, here is another idea.

In the :extras configuration, we can allow a map to be given. The map will take the source file as key and the output file as value. This way, we:

  1. allow users who want to configure paths to explicitly do so by giving a map to extras
  2. allow users who do not care about the URL to have a safe default of using extra-, which won't ever conflict
  3. it also allows folks to customize URLs in any way they want

Would this be preferable to removing prefixes altogether?

@milmazz
Copy link
Member Author

milmazz commented Nov 5, 2015

@josevalim Please correct me if I'm wrong. I understood you to say that we can cover the following scenarios:

Input Output Comment
extras: ["README.md"] output/extra-readme.html Current behavior
extras: %{"README.md": "README"} output/README.html Allow to override the current behavior (lowercase + prefix)
extras: %{"API.md": "extra-api-reference"} output/extra-api-reference.html Allow to override module docs and the API reference (warn the user about this)
extras: %{"tutorial/part1.md": "guides/getting-started"} output/guides/getting-started.html Better URLs organization

Did I understand you correctly? I think that your idea is clever, can clutter a little bit the mix.exs file (there are ways to avoid this), but at the end we can cover all the possible scenarios.

@josevalim
Copy link
Member

@milmazz that's it. :D although we can't support nesting with directories because all assets break (and I don't think it is fixable). What do you think @wsmoak?

@wsmoak
Copy link
Contributor

wsmoak commented Nov 6, 2015

I think no prefix is preferable as the default.

If there is a conflict I think it's likely going to be someone doing it on purpose to try to override the generated content (which does not currently seem to be possible) and either raising or warning is fine... they'll wander over here and ask.

So let someone add the prefix if they need it, or just learn how it works and rename their file to avoid the issue.

Is this how the "Extras Map" proposal would work?

original: extras: [ "path/to/Overview.md", "Installation.md" ]

new: extras: %{ "path/to/Overview.md" => "overview.html", "path/to/Installation.md" => "installation.md" }

Works for me! :)

Reminder that there is more discussion in https://groups.google.com/forum/#!topic/elixir-lang-core/UzhyIhvQsC8

@josevalim
Copy link
Member

My concern about removing "-extra" is that it is a backwards incompatible change and there are some projects already relying on it. I really like the indication it is some extra content and not part of the generated docs. :(

It seems we need to do two things to give the flexibility you need:

  1. Allow the URL to be different from "extra-filename". That's the proposal above. Keep in mind though you likely want to use a keyword list so you keep the order: ["overview.md", "intro.md": "new-introduction"]
  2. Extract the title from the first # in the document as @wsmoak proposed somewhere else

This seems to be enough, so I think we can close this PR. Thanks everyone for the discussion!

PS: @wsmoak, if you are porting Phoenix guides, I think you will eventually need to add extra assets to the docs. Like images and so on. That should be fun to support too. :)

@eksperimental
Copy link
Contributor

@wsmoak I think a collision may happen unintentionally. somebody having a module named "Authors" and a file called "AUTHORS.md" for example

@milmazz
Copy link
Member Author

milmazz commented Nov 6, 2015

My concern about removing "-extra" is that it is a backwards incompatible change and there are some projects already relying on it. I really like the indication it is some extra content and not part of the generated docs.

@josevalim Is this is your main concern I think we should consider the following options:

  • As we did with the –readme option, we can deprecate this behavior because we are before 1.0 😁
  • Being more serious about your concern, suppose for a moment that we decide to get rid of the prefix, then, for each item in the extras option we can create 2 files (for a certain period), one without the prefix with the content on it (as @wsmoak propose) and the other file just use the redirect_template.eex to be backward compatible.

Regardless of the options mentioned before, I think that the best way is to allow the developer to rename the files via mix.exs as they want. So, the Keyword list is a really nice option because will allow to rename and specify the order of the additional files .

@wsmoak
Copy link
Contributor

wsmoak commented Nov 6, 2015

I agree @milmazz -- ExDoc is pre-1.0 and this is the time to put it through its paces and figure out what works best.

I'd really like to see the configuration called "pages" rather than "extras". As I mentioned on the mailing list, from my perspective this is not something 'extra' or incidental to the project, it is "The Documentation".

And I found it confusing to see Pages in the generated HTML and then have to figure out that it came from configuration called "extras".

Maybe "extras" could be deprecated and continue to work as-is for a time, and "pages" will have the new behavior?

@josevalim
Copy link
Member

And I found it confusing to see Pages in the generated HTML and then have to figure out that it came from configuration called "extras".

That's because the "Pages" bit was supposed to be customizable but we never got to it. For example, Phoenix may want to call it "Guides". :)

@milmazz
Copy link
Member Author

milmazz commented Nov 6, 2015

@josevalim @wsmoak I think with the commit 0038081 we can cover all the scenarios:

  • extras: ["README.md", "AUTHORS.md"] keeps backward compatibility. With this proposal, ExDoc will generate a readme.html and extra-readme.html file, the latter will redirect to the former :)
  • Do you want to be in control of the generated files? What can you do?, now you can define something like: extras: ["README.md", "AUTHORS.md": "CONTRIBUTORS"], ExDoc will keep backward compatibility for the README.md file and ExDoc will create a CONTRIBUTORS.html file for you :)
  • If somebody try to do the following: extras: ["AUTHORS.md", "directory/CONTRIBUTORS"], ExDoc will create the CONTRIBUTORS.html file, but this file will be located at the root directory. We hope in the future to offer the possibility to create sub-directories, but first we need to solve some issues related with how ExDoc link its assets (fonts, JS, CSS, images, etc.)
  • Last but not least, the user will receive a message if some file is replaced.

@josevalim
Copy link
Member

@milmazz if we are going to remove the prefix, there is no need to create a page that redirects. As you said, we are before 1.0, so let's just get rid of it. :D

@wsmoak
Copy link
Contributor

wsmoak commented Nov 7, 2015

I tried this out (using the correct branch this time) and I can override the filenames, but it does make for a lot of repetition in the config.

If the "extras-" prefix is going away entirely (yay!) then I'll wait...

Thanks @milmazz -- I wasn't expecting someone to do the work for me. :) Let me know if I can help.

@milmazz
Copy link
Member Author

milmazz commented Nov 7, 2015

if we are going to remove the prefix, there is no need to create a page that redirects. As you said, we are before 1.0, so let's just get rid of it. :D

@josevalim Done!

I tried this out (using the correct branch this time) and I can override the filenames, but it does make for a lot of repetition in the config.

@wsmoak The current proposal does not create redirects anymore. Please tell me what do you think and how we can improve your experience? Also, please remember that you can open new issues :)

Thanks @milmazz -- I wasn't expecting someone to do the work for me.

I'm glad to help.

@milmazz milmazz closed this in #467 Nov 7, 2015
@milmazz
Copy link
Member Author

milmazz commented Nov 7, 2015

It looks like after I merged #467 into master this issue was closed automatically and now I can't reopen this Pull Request because GitHub does not allow it :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants