Added :markdown_processor option to the mix config #766
Conversation
Code blocks are now handled by the markdown modules, and ExDoc doesn't do any post-processing. The modules bundled with ExDoc were updated to reflect this. The goal of this change is to make it easier to use alternative Markdown implementations. This might break compatibility for people that are already usin Markdown extensions.
Docs still missing
Users can now pass the :markdown_option from the mix project config as well as from the app's environment. The app environment overrides the choice in the mix project config.
|
Hello, @tmbb! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
| doc_config_markdown_processor_not_set() | ||
| # MockMarkdownProcessor will raise an exception if used to process files | ||
| # This exception will be our way to tell that someting's not right. | ||
| |> Keyword.put(:markdown_processor, MockMarkdownProcessor) |
There was a problem hiding this comment.
Use a function call when a pipeline is only one function long
| @@ -0,0 +1,84 @@ | |||
| defmodule ExDoc.MarkdownProcessorOptionTest do | |||
| use ExUnit.Case | |||
|
|
|||
There was a problem hiding this comment.
There should be no trailing white-space at the end of a line.
lib/ex_doc/markdown/mocks.ex
Outdated
| def to_html(_, _) do | ||
| # When we get an exception, we know this processor's been used | ||
| # This seems to be the easiest way to ensure we're choosing the right processor | ||
| # without instrumenting the code |
There was a problem hiding this comment.
There should be no trailing white-space at the end of a line.
lib/ex_doc/markdown/mocks.ex
Outdated
| # without instrumenting the code | ||
| raise ExDoc.Markdown.MockMarkdownProcessor.MockMarkdownProcessorError | ||
| end | ||
| end No newline at end of file |
There was a problem hiding this comment.
There should be a final \n at the end of each file.
|
Thanks @tmbb! I think this change is counter-intuitive because we are using a local option to set a global value - therefore I don't think we should merge this as is. However, the other option is to pass the markdown processor every time we need to convert to markdown in the formatters and that may be a change that is too large and maybe even not worth it. Thoughts? |
You're the one who said the app environment was "more local" than the mix, config right? I agree it's a little strange, but it's probably not worth it to pass the markdown processor explicitly. I thought of that when I started working on this, but ultimately decided against. |
|
On one hand, setting the option from the mix config is just a minor cosmetic change and it's probably not worth it to desctructure perfectly good code because of that. So we could simply not merge this (any other approach will probably do the same thing as my code). If we really want this feature, we can pass the paameter around or just decide that one little global variable can't hur that much. In situations like these, the best answer might be to defer to this xkcd strip: https://xkcd.com/292/ :) Now, seriously, your call. What the code is doing now doesn't shock me. But this feature is just a "nice shiny thing", and nothing fundamental. |
tmbb
changed the title
|
Thanks @tmbb, I won't merge it for the reasons above. Btw, I probably expressed myself incorrectly previously, but the project configuration is more local than the application config. Because the project configuration is about your project and the app config applies to |
Ok, then I might have gotten that wrong then. Should I submit a PR that does it the other way around (having the project's config override the app's env)? That is probably even easier and less contrived than what we have now :) Or you don't want this feature at all? |
|
I think changing the order wouldn't remove the fact we would need to rewrite all calls to markdown... so better to leave things as they are IMO. :) |
|
No, I would still set the apps env, just in the "right order". But this is really not a priority, so let's leave it as it is. |
|
Just commenting here in case you want to reopen the issue. |
|
/me is curious... Why not set it in the mix.exs file via a new option there that ex_doc reads (if mix exists). It's not like it is a runtime configuration after all (well, I guess someone could include ex_doc at runtime, but even then they could set it via a command in that case)... Or are you looking for a way to have the decision propagate down to dependents (things that depend on ex_doc or some other library there-of)? |
|
@OvermindDL1 yes, now i agree. most of the use cases are via the command line, so we shouldn't sacrifice the convenience of doing that via mix.exs because of a very uncommon use case. |
That is exactly what this PR does. |
|
Since this branch is no longer available, I pushed a similar patch to master: 69dd28e Thank you! |
|
Sorry for that, at the time I deleted it to get a cleaner history for some reason... I didn't think we'd be using it. |
|
@tmbb no problem at all! |
The changes are very simple and they work well both in practice and in ExUnit tests.
The problem is that the only way I can think of testing this is somewhat contrived. The idea is that I created a mock Markdown processor that raises an exception when called, and use the exception as a a way to decide if the right markdown processor was called.
If you think there is a better way of testing this, I can refactor it.