Allow management command to ignore missing file #191
Conversation
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
==========================================
+ Coverage 96.49% 96.59% +0.09%
==========================================
Files 5 5
Lines 257 264 +7
==========================================
+ Hits 248 255 +7
Misses 9 9
Continue to review full report at Codecov.
|
|
aaaand it contains a misprint and error that does not exist in python2. So prob not such a good idea. |
|
python2 incompatibility bothers me but I don't see any other clean way to do it. |
There was a problem hiding this comment.
Cool @marojenka , good work!
I left you some comments about a couple small things. My bigger request would be to add a test ;)
| if not igrnore_missing: | ||
| raise | ||
| else: | ||
| logger.error(str(e)) |
There was a problem hiding this comment.
Please don't use the logger here. The command should write to stderr.
If you would want to log an exception, you should actually log exception and always provide your own message. The original exception and stack trace will be included anyways. You're log should provide additional information to why the exception was not raised.
There was a problem hiding this comment.
Anyhow, in this case please write to self.stderr.
There was a problem hiding this comment.
I kinda was too used to trowing everything to the logger without additional thought. Yeah, I will change that a bit later.
| @@ -8,6 +9,8 @@ | |||
|
|
|||
| from stdimage.utils import render_variations | |||
|
|
|||
| logger = logging.getLogger() | |||
There was a problem hiding this comment.
I explain further down why I don't think using a logger is a good idea. But as a side note here:
I would recommend to use the package name. That way people have only a single logger per package, that they can configure in their projects.
There was a problem hiding this comment.
I also don't know the default, I think you are getting the root logger here, where I think you wanted __name__. Anyhow, check my comment further down about stderr.
There was a problem hiding this comment.
yeah u are right, I just missed that.
quality code
| @@ -24,8 +27,16 @@ def add_arguments(self, parser): | |||
| default=False, | |||
| help='Replace existing files.') | |||
|
|
|||
| parser.add_argument('--igrnore-missing', | |||
There was a problem hiding this comment.
I would recommend to add a short version too, -i.
| action='store_true', | ||
| dest='igrnore_missing', | ||
| default=False, | ||
| help='Do not rise exception on missing file ' |
There was a problem hiding this comment.
How about something that does not include a negation:
Ignore and skip missing file errors.
Up in the help you might want to document, that the command will exit with non-zero exit code if a file is not found.
| render_variations(**kwargs) | ||
| except FileNotFoundError as e: | ||
| if not igrnore_missing: | ||
| raise |
There was a problem hiding this comment.
I would recommend to cast to a CommandError and provide a more descriptive error message, eg:
raise CommandError("Original file was not found, terminating command. Use --ignore-missing to skip this error.") from e
I did some test 024a340#diff-59262c12118a93348565a41ab7f6ce42R86 the rest are reasonable comments, thanks. |
That doesn't look bad, but the commit is not in your PR.
thx, I try my best :) |
|
It's my last (badly named) commit in this request tho |
Logger is removed from rendervaraiations command. CommandError is raised (saving original error stack) when source file is missing and nothing is done if --ignore-missing option is set. Add short version of --ignore-missing option: -i. Add test for it.
|
it was some time but here is my changes to your comments. |
|
Released in 3.2.0 |
QOL fix allowing rendervariations command to process missing files by logging error instead of complete break.