Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
garfieldnate opened this issue Apr 25, 2024 · 11 comments · Fixed by #230
Closed

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

garfieldnate opened this issue Apr 25, 2024 · 11 comments · Fixed by #230
Labels
documentation Documentation is required enhancement New feature or request fixed A fix has been submitted

Comments

@garfieldnate
Copy link
Contributor

garfieldnate commented Apr 25, 2024

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.

@garfieldnate garfieldnate changed the title issues should be printed at WARN level during mkdoc serve/build all issues should be printed at WARN level during mkdoc serve/build Apr 25, 2024
@garfieldnate garfieldnate changed the title all issues should be printed at WARN level during mkdoc serve/build all issues should be printed at WARN level during mkdocs serve/build Apr 25, 2024
Copy link

Welcome to this project and thank you!' first issue

@fralau fralau added enhancement New feature or request info required Further information is requested documentation Documentation is required labels Apr 27, 2024
@fralau
Copy link
Owner

fralau commented Apr 27, 2024

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 pushed a commit that referenced this issue Apr 27, 2024
    - Generated a warning so as to make the build fail
      in strict mode (`mkdocs build --strict`)
    - Updated documentation of Advanced usage and Troubleshooting
      to clarify this.
    - Bumped revision number.
fralau pushed a commit that referenced this issue Apr 27, 2024
    - Generated a warning so as to make the build fail
      in strict mode (`mkdocs build --strict`)
    - Updated documentation of Advanced usage and Troubleshooting
      to clarify this.
    - Bumped revision number.
@fralau fralau added the fixed A fix has been submitted label Apr 27, 2024
fralau pushed a commit that referenced this issue Apr 27, 2024
@fralau
Copy link
Owner

fralau commented Apr 27, 2024

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

@garfieldnate
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@garfieldnate
Copy link
Contributor Author

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 pushed a commit that referenced this issue Apr 28, 2024
@fralau
Copy link
Owner

fralau commented Apr 28, 2024

@garfieldnate Try now?

@garfieldnate
Copy link
Contributor Author

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
Copy link
Owner

fralau commented May 13, 2024

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.

@fralau fralau added solved The issue has been satisfactorily solved and removed info required Further information is requested solved The issue has been satisfactorily solved labels May 13, 2024
@garfieldnate
Copy link
Contributor Author

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
Copy link
Owner

fralau commented May 13, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation is required enhancement New feature or request fixed A fix has been submitted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants