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

Improve error/warning handling in Config #536

Closed
waylan opened this issue May 19, 2015 · 9 comments
Closed

Improve error/warning handling in Config #536

waylan opened this issue May 19, 2015 · 9 comments
Labels

Comments

@waylan
Copy link
Member

@waylan waylan commented May 19, 2015

While working on moving the markdown extension handling to the config, I noticed a few oddities with how errors and warnings are handled within the config. And taking a closer look at the code I see even more. I could just do a PR, but wanted some feedback on where you want to take this.

  1. I noticed that you need to hardcode the config key into any warnings (see this example) . This is bad as the same ConfigOption class could be used for more than one key (for example, if the Type or URL types raised a warning). base.Config._validate does the right thing with errors and uses the current key (see L51). The same thing should happen for errors two lines above.
  2. Why don't warnings actually use warnings? Why not follow the pattern used for errors and define a ValidationWarning which subclasses warnings.Warning. Then within the code call raise ValidationWarning('some message...') and in base.Config._validate add a catch for warnings in addition to the current catch for errors. Using the same API for both warnings and errors seems more sensible to me (and easier to remember).
  3. Why are the errors being obscured? Only the message from the error is saved (see L51) and then later a new error is raised (see L148). Wouldn't it be better if the errors themselves were saved and re-raised? It would certainly help with debugging. Yes, I realize that if multiple errors were raised, only one could get re-raised. Not sure what the best solution is here for errors. Maybe log all the errors and then raise the first one? At least that way the user can work through them one at a time if the traceback is needed to figure out what went wrong.
@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 19, 2015

1, I noticed that you need to hardcode the config key into any warnings

Yeah, that's bad. I was planning on changing to have the same method signature as post_validation which would make the key available.

2 Why don't warnings actually use warnings?

Because how do you add multiple warnings with warnings.Warning?

3 Why are the errors being obscured?

Because I don't want config errors to be the concern of anything outside of mkdocs.config and I also want to make sure we list all errors, I hate tools that show you one error at a time and you need to keep running it and fixing them one by one.

@d0ugal d0ugal added the Cleanup label May 19, 2015
@waylan
Copy link
Member Author

@waylan waylan commented May 19, 2015

2 Why don't warnings actually use warnings?

Because how do you add multiple warnings with warnings.Warnings?

Ah, sorry, you want to use warnings.warn not raise Warning. Try running this code:

import warnings
print 'Before warnings'
warnings.warn('A first warning')
print 'Between warnings'
warnings.warn('A second warning')
print 'Atfer warnings'

The output will be:

$ python foo.py
Before warnings
foo.py:4: UserWarning: A first warning
  warnings.warn('A first warning')
Between warnings
foo.py:6: UserWarning: A second warning
  warnings.warn('A second warning')
Atfer warnings

You can have as many warnings as you want and code execution never stops.

Of course, they don't need to be used. I was just thinking a consistent API for both warnings and errors would be nice. But as we can't "raise" warnings without stopping code execution, I guess it doesn't really matter for this use case.

3 Why are the errors being obscured?

Because I don't want config errors to be the concern of anything outside of mkdocs.config and I also want to make sure we list all errors, I hate tools that show you one error at a time and you need to keep running it and fixing them one by one.

Yes, but I loose the original traceback when debugging, which, in my opinion, is much more important. That is why I suggest passing the errors themselves (not just the messages). Then, while you can still list all errors, you can re-raise at least one of them (rather than a new error) and get the traceback. Worst case scenario: I fix all errors listed expect the one I can't make sense of, rerun and get the traceback for that one outstanding error.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 19, 2015

Perhaps if the --verbose flag is provided we can error on the first problem and show the traceback?

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 20, 2015

I forgot to add, I've been doing some further work on configs for #144, and it covers point 1. that you mentioned here.

It is still a work in progress. https://github.com/d0ugal/mkdocs/tree/deprecated-config

@waylan
Copy link
Member Author

@waylan waylan commented May 21, 2015

I've been thinking about this some more and I continue to believe that the error instance needs to be passed through (not just the message string). Note that the logging module actually supports recording the error along with the message (see the explanation for the exec_info kwarg on Logger.debug). Then with increased verbosity, perhaps the additional info could be output for each item.

Also, the new ConfigurationError that is raised here and its traceback is confusing. It appears after the list of warnings and errors that actually matter. And in fact, the user may need to scroll up to see the warnings and errors that matter. Why not just exit with exit code 0 (maybe use click.Abort)? The point it, the current trace_back is misleading and masking the actual problems that need to be resolved. As it serves no useful purpose, I would remove it.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 21, 2015

Sure, I'd be happy to see this be improved. So long as it doesn't change anything behaviourally and we display all errors in one go. Go for it.

... it's just relatively low in my priorities. There are far uglier areas than the config now.

@waylan
Copy link
Member Author

@waylan waylan commented May 21, 2015

Additionally, the ConfigurationError which is raised when config['strict']==True` for warnings [here][1], prevents the errors from ever being logged. Of course, any errors will always exit, so the warnings need to be logged first. Perhaps L141 should be changed as follows:

-         if config['strict']:
+         if config['strict'] and len(errors) == 0: 

Again, the raised error should be replaced with a simple exit with exit code 0 for the reasons stated in my previous comment.

Although, it occurs to me that another solution would be to log all warnings and errors when they happen, rather than gathering them and logging them later. In other words, each individual Option could log the warnings in their validate/run_validation method rather than calling self.warnings.append. The errors could be a little trickier as then stop execution when raised, but its not impossible and would provide a more consistent API for errors and warnings.

@d0ugal
Copy link
Member

@d0ugal d0ugal commented May 21, 2015

That's a good catch with regards to strict. I'll fix that. Thanks!

Although, it occurs to me that another solution would be to log all warnings and errors when they happen[...]

I started with this approach early on, but didn't really like it. I don't think the change is worth it for a consistent API. In some ways, I like that they feel different because the use case is different enough.

@waylan
Copy link
Member Author

@waylan waylan commented May 21, 2015

Although, it occurs to me that another solution would be to log all warnings and errors when they happen[...]

I started with this approach early on, but didn't really like it. I don't think the change is worth it for a consistent API. In some ways, I like that they feel different because the use case is different enough.

Fair point. It occurred to me there might be a reason for it. As you started with that route first and abandoned it, I'll take your word for it.

Sure, I'd be happy to see this be improved. So long as it doesn't change anything behaviourally and we display all errors in one go. Go for it.

I'll see what I can work up when I find the time. Although it will probably wait till after #539 hits.

waylan added a commit to waylan/mkdocs that referenced this issue May 28, 2015
Also ensure all debug messages are logged. And exit without
creating a new, unhelpful traceback by raising SystemExit.
Note that `sys.exit` raises SystemExit. However, by raising it
explicitly, we can also output a helpful explaination of why
the program exited. The program exists with an exit code of 1
to indicate is failed.

Fixes mkdocs#536.
@d0ugal d0ugal closed this in #569 May 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants