-
Notifications
You must be signed in to change notification settings - Fork 4
Catch exceptions in the only handling #7
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
base: master
Are you sure you want to change the base?
Conversation
| if not env.app.builder.tags.eval_condition(self.arguments[0]): | ||
| return [] | ||
| except Exception as err: | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a global variable of the file, or a class-member, or property or whatever. Having it here in the except only, will force new logs to copy this line around...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a global variable
+1. And please initialize it as logger = sphinx.util.logging.getLogger(__name__) to avoid confusion of what it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done this now.
Sorry I missed this comment.
| logger.warning(('exception while evaluating only directive expression: %s'), err, | ||
| location=(self.env.docname, self.lineno)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure we just want to log a warning and continue building? There probably wont be any html/latex generated for this directive due to this syntax error. And not every rst-write cares too much about terminal warnings. Two suggestions:
- or 1, we still fail the sphinx-build but give more details on where the syntax error is located
- or 2, we add some warning in the generated html/latex as well stating there was a syntax error,
for option 1, we could derive a specific Exception class where we added more details. For an example see https://github.com/melexis/sphinx-traceability-extension/blob/master/mlx/traceability_exception.py#L38-L60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stein,
I intended to raise. So I corrected this already.
For this exception class. I do not know whether there is much added value at this point.
This sphinx_selective_exclude is only 1 file so it is still clean like it is added now.
Usually I add this class the second time I add the same code.
Piet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"exception while evaluating ... expression" looks like an logger.error() to me, not logger.warning(). Extra confirmed by the fact that you raise after it (so it's treated as kinda-fatal, which is not ideal, but ok).
|
Hello @pfalcon, Can you merge this one? Regards, |
| return [] | ||
| except Exception as err: | ||
| logger = logging.getLogger(__name__) | ||
| logger.warning(('exception while evaluating only directive expression: %s'), err, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be "exception while evaluating 'only' directive expression", otherwise it's confusing. Parens around message are superfluous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this
|
Sorry, I'm too busy with the real life currently, and preparing for travel, so won't be able to look into this in detail (at least) until 2nd half of November. In the meantime, a few suggestion based on a quick look. Please squash any fixes into the original commit. Also, if you didn't yet do that, please do "git log" and follow the same format for the commit message. Also, can you please add an example of erroneous syntax and corresponding error message generated with this patch? Thanks! |
Co-authored-by: Jasper Craeghs <[email protected]>
Co-authored-by: Jasper Craeghs <[email protected]>
Give rst line and file information upon exceptions in sphinx_selective_exclude
Example syntax error:
With this fix the exception info:
Fixes #6