-
Notifications
You must be signed in to change notification settings - Fork 8
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
Do not ignore exception while reading configuration file #260
Conversation
Please note: I have only read the code, but not tested it Could you create a unit test in which the configuration file exists but is not valid json? Also, I don't really like how we are using |
I tested this pull req with an empty json configuration file an a malformed one (i.e. a file I am going to merge this pull request, as far as there are no objections, and would open another issue to signal the need for a proper test unit, as signaled by @boyska |
On 03/16/2016 03:28 PM, Blallo wrote:
please give me some more time to analyze it. |
Sorry @ael-code, I don't get it. Do you think there is something wrong in your pull request? =) |
try: | ||
with open(fname) as buf: | ||
conf = json.load(buf) | ||
return conf | ||
except Exception: | ||
log.warning('Error loading config file', exc_info=1) |
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.
adding fname
to this warning would probably fix the problem raised by @leophys
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 said a stupid thing. You removed that exception handling. Instead, you only catch EnvironmentError
, which is what happens when, for example, you don't have permissions.
You should add an except ValueError
clause
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.
On 03/16/2016 04:12 PM, BoySka wrote:
In conf/config_utils.py
#260 (comment):try: with open(fname) as buf: conf = json.load(buf) return conf
- except Exception:
log.warning('Error loading config file', exc_info=1)
I said a stupid thing. You /removed/ that exception handling. Instead,
you only catch |EnvironmentError|, which is what happens when, for
example, you don't have permissions.
Hahhah I've tried exactly the same thing when I've coded it. This is the
reason why the name it is not printed here.
You should add an |except ValueError| clause
the function do not make use of it.
substituted both logging.warning with a unified well written exception. One test was changed.
0bf9e2f
to
638d4ad
Compare
using the wrapper we have cleaner code and consitent behaviour among all commands
638d4ad
to
dfb4044
Compare
Thanks for the very clear explanation. Unfortunately it could be the case that conf file isn't the only json involved. I've added a specific error log line in which the path of the file is printed along with the reason why it is malformed.
Done. We now have:
I really don't like this division but exception handling of Click sucks. This is the best solution I've found.
Done |
@leophys any news on this? |
Except for some cleanup and refactoring the main change is bring by the second commit, and it should fix #255.
It does not matter how the configuration file path is given (env, cli option, etc...), if there is an error while opening that file, an exception is raised.
The cli will print the stack trace if in debug mode otherwise it will print a colored one-line message.
There are no test on cli side, so please make some manual test.