Add plugin error exception and event

[pawamoy]

This PR adds a PluginError exception to the mkdocs.exceptions module to allow plugin writers to cleanly abort a build without showing a traceback.

It also adds an on_build_error event that is triggered by such PluginError exceptions, that we catch in the build command of the mkdocs.commands.build module (most of the diff in this module is just indentation in between the try/except, I could not reduce the noise). This event allows to clean things up if needed, just before aborting the build.

I wondered if I should not rename the event on_plugin_error, but to me it makes more sense to call it a build error, because it could be triggered in other places as well. Also, if you think the event doesn't have its place in this PR, I'll gladly remove it so we can discuss it a bit more before opening (or not) another PR for it.

Related: #2082 (the issue also asks for a logging error counter, as well as a get_logger utility function, so this PR does not resolve it entirely).

TODO:

• add docs: I documented the on_build_error event, but not the PluginError since I couldn't find an existing page documenting MkDocs exceptions. It's mentioned in the on_build_error documentation though.
• add tests (any advice?)

[waylan]

Looks like a great start. I know the noise in the diff is annoying, but it cannot be helped.

I would suggest creating a new section to the Plugins page and document the PluginError there. Perhaps call it Handling Errors or something to that effect. Of course, it should link to the event and perhaps provide more context for when you would use the event. The event should probably link back to this section as well.

Tests for the Plugin API are in mkdocs/tests/plugin_tests.py. Notice that most of the tests there make use of simple events. I suggest adding an event (or two) that raises a PluginError and a build_error event. Then add a few tests that verify they are being called properly.

I'm not crazy about the name build_error as it is specific to the PluginError. The name should reflect that it handles "plugin errors." Of course, if PluginError was a subclass of BuildError and both were handled by the event, then that would be different. But, as I understand it, that is not the plan (although I suspect perhaps you are hoping future additions go that way). I'm open to suggestions here.

[pawamoy]

Thank you very much for your review! I will add the Handling Errors section to the documentation, and I'll happily let you suggest improvements over the text because English is not my native language and I'm not the best technical writer 🙂

Of course, if PluginError was a subclass of BuildError and both were handled by the event, then that would be different. But, as I understand it, that is not the plan (although I suspect perhaps you are hoping future additions go that way). I'm open to suggestions here.

This is a very good idea. I can create this BuildError and make PluginError a subclass of it. I have no intentions to modify or suggest modifications on how MkDocs is handling its own errors, I just felt that the name plugin_error for the event would be too ambiguous ("what's its purpose??"), whereas build_error was very explicit about what it does.

[pawamoy]

• I tried to be clearer in the description of the event.
• I added BuildError, and PluginError is a subclass of it
• Both types are caught with except BuildError in the build command
• I added a test in build_tests.py, because plugins tests were not actually "building" and this was required to check if the exceptions were correctly caught and handled

[waylan]

Look good. With just have a few minor adjustments this should be good to go.

Note, I have not carefully reviewed the documentation. I'll do a more thorough review and make my own edits when I have the time.

[pawamoy]

Note, I have not carefully reviewed the documentation. I'll do a more thorough review and make my own edits when I have the time.

Of course, no problem 🙂

[waylan]

I reviewed the docs and made some adjustments in 6568cc4. @pawamoy what you had was a great start. Its always easier to work off of something than to start with nothing.

Now that I'm more familiar with the docs, I have a few thoughts:

1. Perhaps we should be catching MkDocsException instead, which is more inclusive than BuildError.
2. I keep wondering why we need the reraise argument. Just let a non-MkDocs specific error bubble up. Then I remember that only the MkDocs specific errors result in the on_build_error event being called. I wonder if we need to make that more clear in the docs.

[waylan]

Also, if you see anything in the docs that I missed, please feel free to make the changes.

[pawamoy]

Hey @waylan, your changes to the docs are great, thanks a lot 🙂

You raised two good points indeed.

Perhaps we should be catching MkDocsException instead, which is more inclusive than BuildError.

I guess it makes sense to only catch BuildError because it happens in the build command. ConfigurationError are caught somewhere else for example. However if you feel like we should catch MkDocsException, I think it wouldn't hurt, so I'm fine with it.

I keep wondering why we need the reraise argument. Just let a non-MkDocs specific error bubble up.

True.

Then I remember that only the MkDocs specific errors result in the on_build_error event being called. I wonder if we need to make that more clear in the docs.

Yes we should tell that in the docs as it could be confusing ("error raised but event not called?"). Maybe we should call the event for any error then? And re-raise only if it's not a BuildError (subclass / instance / subclass instance) error.

[Artnoc1]

This PR adds a PluginError exception to the mkdocs.exceptions module to allow plugin writers to cleanly abort a build without showing a traceback.

It also adds an on_build_error event that is triggered by such PluginError exceptions, that we catch in the build command of the mkdocs.commands.build module (most of the diff in this module is just indentation in between the try/except, I could not reduce the noise). This event allows to clean things up if needed, just before aborting the build.

I wondered if I should not rename the event on_plugin_error, but to me it makes more sense to call it a build error, because it could be triggered in other places as well. Also, if you think the event doesn't have its place in this PR, I'll gladly remove it so we can discuss it a bit more before opening (or not) another PR for it.

Related: #2082 (the issue also asks for a logging error counter, as well as a get_logger utility function, so this PR does not resolve it entirely).

TODO:

* [x]  add docs: I documented the `on_build_error` event, but not the `PluginError` since I couldn't find an existing page documenting MkDocs exceptions. It's mentioned in the `on_build_error` documentation though.

* [x]  add tests (any advice?)

[pawamoy]

Hello all. Sorry for coming back to this so late.

I implemented what was said in the previous comments and suggestions:

Yes we should tell that in the docs as it could be confusing ("error raised but event not called?"). Maybe we should call the event for any error then? And re-raise only if it's not a BuildError (subclass / instance / subclass instance) error.

The on_build_error event is now triggered for any exception caught by MkDocs. If this exception is an instance of BuildError (true for instances of PluginError as well), then MkDocs will raise a SystemExit with the error message, and will not print any traceback. Any other exception is raised again.

The docs were updated accordingly (they were greatly simplified by this change!).

@waylan what do you think?

[oprypin]

FYI the whitespace change can be hidden from the diff during review.
https://github.com/mkdocs/mkdocs/pull/2103/files?diff=unified&w=1

Thanks for the PR, I'm excited about it :)

[pawamoy]

Hi @waylan, thank you for putting this in the 1.2 milestone 🙂 !

Is there anything I need to update (I see the label is still here)? Happy to help in any way.
/cc reviewers @Stanzilla @Artnoc1

[waylan]

I just need to find the time to properly review this.

[pawamoy]

Noted, thank you very much!

[waylan]

I finally found some time to do a final review. This looks good! 👍

@pawamoy if you rebase and resolve the the conflict, I'll merge this.

[oprypin]

Shouldn't a merge commit be preferred over a rebase and force-push? So much easier to follow the review history then

[waylan]

@oprypin you make a good point. In any event, the conflict needs to be resolved.

[pawamoy]

Thank you for the review @waylan! I've resolved the conflicts, it should be good now.

[waylan]

Oh sorry, I just realized this is missing an addition to the release notes.

[pawamoy]

Should I take care of it? Not sure if I should add it in "Major additions" or "Other changes"? EDIT: ah, you said "missing an addition", here's my answer.

[waylan]

Yes, this is adding a new feature so we should highlight it in the Major additions section.

[pawamoy]

Just added the release notes. As always, feel free to change the wording or even the whole paragraphs!