fralau / mkdocs-macros-plugin

Create richer and more beautiful pages in MkDocs, by using variables and calls to macros in the markdown code.
https://mkdocs-macros-plugin.readthedocs.io
Other
321 stars 50 forks source link

Add an option to raise exception if render fails #133

Closed allebacco closed 2 years ago

allebacco commented 2 years ago

The current behavior of the MacrosPlugin.render() method when the template rendering fails is to return the exception message as the rendered text.

This behavior is convenient when writing the mkdocs pages, but it is less useful when documentation is compiled in a CI/CD environment.

My proposal is to add an option to raise an exception when the rendering fails, in this way the CI/CD process can fail. Otherwise the compiled documentation will miss the relevant information because of the error message of the macros plugin.

The option could be named strict and should be false to keep the current behavior of the plugin.

Thanks to mkdocs 1.2, the option could then be configured with an environment variable like:

plugins:
  - search
  - macros:
      module_name: my_module
      strict: !ENV [ENABLE_STRICT_MODE, false]

In this way, local execution will not raise exception, but execution within CI/CD pipelines could raise exceptions in case of rendering errors.

github-actions[bot] commented 2 years ago

Welcome to this project and thank you!' first issue

fralau commented 2 years ago

Thanks a lot! Yes, it is true that the choice of rendering the template interpretation error itself, was to help debugging.

If my understanding is correct, you are in a CD/CI context, and you wish to set an option that instructs mkdocs-macros to fail in case of rendering error (as e.g. in compilation); and ideally, to provide a return code so that the automation script/tool could pick it up and initiate some adequate exception processing?

At a first glance, it makes sense to me 🤔 .

If yes (and to keep things simple) could something more or less like this work?

plugins:
  - search
  - macros:
      module_name: my_module
      fail_on_error: true
allebacco commented 2 years ago

@fralau Yes, that's will be optimal. Thanks

fralau commented 2 years ago

All right, I am implementing it this way:

plugins:
  - search
  - macros:
      module_name: my_module
      on_error_fail: true

In case of macro syntax or rendering error the return code will be 100.

Would that work for you?

Just curious: what would be the meaning of !ENV [ENABLE_STRICT_MODE, false]? Is it something specific to your Python YAML implementation?

allebacco commented 2 years ago

It is ok for me, thanks.

The !ENV syntax is from mkdocs 1.2 configuration: https://www.mkdocs.org/user-guide/configuration/#environment-variables

fralau commented 2 years ago

It is ok for me, thanks.

Perfect.

The !ENV syntax is from mkdocs 1.2 configuration: https://www.mkdocs.org/user-guide/configuration/#environment-variables

I didn't know: that's cool (and can be handy)!

fralau commented 2 years ago

@allebacco Could you let me know if the new feature works for you?

allebacco commented 2 years ago

The behavior is the expected one: if an exception is raised during template rendering, mkdocs exits.

However, I am suggesting a different implementation. Instead of

exit(ERROR_MACRO)

I am suggesting to use something like:

raise RuntimeError("Error during parsing template") from e

which will raise an exception like:

ERROR    -  Error reading page 'dnm/index.md': error during parsing template
Traceback (most recent call last):
  File "/doc_dir/.venv/lib/python3.8/site-packages/mkdocs_macros/plugin.py", line 480, in render
    return md_template.render(**page_variables)
...
    result = func(*args, **kwargs)
  File "/doc_dir/catalog/macros/macros.py", line 80, in function_with_exception
    raise RuntimeError("This is an expected error")
RuntimeError: This is an expected error

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/doc_dir/.venv/bin/mkdocs", line 8, in <module>
    sys.exit(cli())
  File "/doc_dir/.venv/lib/python3.8/site-packages/click/core.py", line 1128, in __call__
    return self.main(*args, **kwargs)
...
    self._raw_markdown = self.render(markdown)
  File "/doc_dir/.venv/lib/python3.8/site-packages/mkdocs_macros/plugin.py", line 508, in render
    raise RuntimeError("error during parsing template") from e
RuntimeError: error during parsing template

With the suggested implementation, we could have more details about the error. In particular we could know that it happens during macros rendering, but by looking at the original exception, we will know the reason of the error.

The idea is to use the raise ... from ... to chain exceptions.

Please, do not consider my suggestion mandatory, the current implementation is working without problems, thanks.

fralau commented 2 years ago

I could certainly do that, which would actually be simpler in terms of code 🤔 ... since the exit instruction occurs after errors were raised.

My view was principle of economy: as long as the markdown file, error description and line number are properly described in the output, should that not be sufficient to diagnose and correct the error? How that error was found (as in an intepreter or compiler), would seem additional information, and not very useful.

Or is some key piece of information missing in the output?

allebacco commented 2 years ago

No, my solution is over-engineered with respect to the current problem. Even a raise alone is enough for me.

The main issues that I see is that by using exit, the plugin is choosing a very strong behavior, while and exception is less strong and leave for exception management to mkdocs.

fralau commented 2 years ago

🤔 That's true in theory. Do you have an use case in mind, where mkdocs would do something about a macro exception, except failing?

allebacco commented 2 years ago

No, actually I have no use case for that.

fralau commented 2 years ago

That's good. Unless you don't feel this is right, I would leave it this way and close this issue.

If there is a new situation where this solution does not work, we could reopen the discussion?

allebacco commented 2 years ago

I am ok with the current implementation, there is no need to refactor it, now. You can close the issue. Thanks for the support.