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
318 stars 50 forks source link

all issues should be printed at WARN level during mkdocs serve/build #226

Closed garfieldnate closed 3 months ago

garfieldnate commented 3 months ago

When you serve a site that triggers errors in the Macro plugin, the errors are printed at the INFO level instead of at the WARN level. A simple example would be:

def define_env(env):
    @env.macro
    def my_var(file_name):
        raise ValueError("foo")

If you use {{my_var}} somewhere, the output from mkdocs serve will have an INFO message that looks like this:

INFO    -  [macros] - ERROR # _Macro Rendering Error_
...

In addition, if you use an undefined variable, unless you've set on_undefined to strict, you don't get any output at all!

In my opinion, every issue that the macros package knows about should be printed at the WARNING level by default. Authors can then see them and act on them, and can also choose to fail the mkdocs serve and mkdocs build commands with the -s (strict) flag, which we use in CI.

I do see the documentation on how to use on_error_fail to fail the build for errors (which doesn't apply to undefined variables unless on_undefined is also set to strict!), but MkDocs already has some built-in functionality for letting authors inspect issues and fail builds with warnings, so I would find the macros plugin easier to use if it took advantage of this functionality directly. It would be less configuration and a lower likelihood of accidentally deploying a broken site.

github-actions[bot] commented 3 months ago

Welcome to this project and thank you!' first issue

fralau commented 3 months ago

Thank you for your observation. The question of proper processing of warnings and errors becomes increasingly important, now that Mkdocs-Macros is being more used more and more in automated deployment contexts.

The fact that messages signaling a Jinja2 error were passed as INFO instead had escaped my notice. The point that if WARNING was used instead, it would (virtuously) make the build fail on mkdocs build -s, is well taken.

Concerning the processing of errors, I gave attention to that issue by allowing the person who configures the project to regulate how they want errors to be processed.

There is a certain logic behind the rendering of variables, which is connected to Jinja2.

The fact that unknown variables could be not rendered, is documented here.

However, it should not happen: I used keep as default, to avoid that case when variables are being "eaten". So if you say that the variable is not rendered by default, it might be a bug (or an older version on your site). We should start from this: which version of MkDocs and Mkdocs-Macros are you using?

Finally, you are correct: on_undefined: strict is the choice that should be applied for website projects with automated deployment (probably I should update the Advanced usage page, to state that clearly).

fralau commented 3 months ago

This issue has been fixed (on Github). Can you check whether it works for you?

garfieldnate commented 3 months ago

Sorry for the misunderstanding; when I said there is not output at all for undefined variables by default, I meant in the logs, not in the generated site source. As far as I can tell, the site generation features work as intended, and my only suggestion is changing how logging works.

For my workflow, I prefer to build my project locally with strict at least once before pushing and letting CI deploy the site. That way I never check in something that will not properly render and fail the deployment step in CI.

I think the error configuration logic for variables that you have implemented is thoughtful and perfectly fine. I just want potential issues to be logged as warnings, since that's where I'm already looking for any potential issues on my site. As it currently stands, if I follow the basic configuration in this repository's README and simply add macros to my plugins list, then I'm not guaranteed to have a perfectly clean site if I build with -s.

garfieldnate commented 3 months ago

Oh you fixed it already :D That was fast! I'll pull and test

garfieldnate commented 3 months ago

Hmm, I wasn't able to get it working 🤔

I changed my dependency to git+https://github.com/fralau/mkdocs-macros-plugin.git to get the repository version.

Then if I put this in main.py:

def define_env(env):
    "Defines macros for Mkdocs-Macros"

    @env.macro
    def foo(file_name):
        1/0

Then use the {{foo}} variable somewhere, I get this output:

INFO    -  [macros] - ERROR # _Macro Rendering Error_

          ...

           _ZeroDivisionError_: division by zero

           ...

Unless on_error_fail: true is set in the macros settings in mkdocs.yml, this does not result in the build failing. It looks like macros is printing at the error level, but then mkdocs wraps it and interprets it as an INFO-level log?

I'm also not getting any logs at all when I use an undefined variable and don't have on_undefined set to anything in mkdocs.yml.

fralau commented 3 months ago

@garfieldnate Try now?

garfieldnate commented 3 months ago

Okay, an error raised in main.py does indeed now print at the WARNING level, so building/serving with -s fails out in that case. Great!

However, if I use a variable that's not defined, I still don't get any warnings.

fralau commented 3 months ago

Thanks for your confirmation! Your issue helped make MkDocs-Macro's behavior more logical in case of warning.

If you want to catch an undefined variable, see how you can alter the behavior of Mkdocs-Macros.

garfieldnate commented 3 months ago

No problem, thank you for creating the plugin!

  1. This "debug" mode reduces cognitive overhead in case of misspelled variable. Anyone will be able to detect this error

This requires hand-checking the pages that use variables, which a user won't necessarily do if it builds cleanly with -s, which normally indicates that everything is squeaky-clean. Emitting warnings for undefined variables would be the most expected behavior and give users the least friction to using the plugin, in my opinion.

  1. Other plugins than mkdocs-macros make use of jinja2 variables

That is a good point and is actually really annoying 😆 I guess the fix would be to put the variables into a namespace, e.g. {{ macros.my_var }}. This would obviously be a breaking change, so I understand it's impossible to fix now without a major version bump.

So I guess perhaps my one and only suggestion at this point would be to mention this bit of configuration in the Declaration of plugin section of the readme, which is probably the only piece of documentation most users look at before they start using the plugin.

fralau commented 3 months ago

Thanks for your PR. In that way, users will know what to do.