Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Clarify config error message #150

Merged
merged 2 commits into from
Jan 6, 2018
Merged

Clarify config error message #150

merged 2 commits into from
Jan 6, 2018

Conversation

ixje
Copy link
Member

@ixje ixje commented Jan 4, 2018

What current issue(s) does this address?, or what feature is it adding?
Currently when you try to configure an non existing event type, or simply mistype it you'll get a partially confusing error message.

neo> config sc-evnt on
cannot configure sc-evnt
Try 'config log on/off'
neo> config log on
cannot configure log
Try 'config log on/off'

The "cannot configure " message makes sense, but the one following that suggests you can type config log on or config log off which you cannot.

How did you solve this problem?
I changed the error message to:
print("Invalid config target: %s", what)

I believe that without giving a suggestion on what to do next, users will likely try help first and see the list of available config commands.
How did you make sure your solution works?
ran unit tests. no errors

Did you add any tests?
no

Are there any special changes in the code that we should be aware of?
no

@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.132% when pulling 0dda123 on ixje:clarify_config_error into 86a62df on CityOfZion:development.

Copy link
Contributor

@metachris metachris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the error message, and just change it to what users can actually use: config sc-events on|off and config debug on|off.

Also we've disabled E501 (max line length) in setup.cfg, so you wouldn't need to change the formatting on those lines.

@ixje
Copy link
Member Author

ixje commented Jan 4, 2018

I specifically choose to keep the error message generic so you don't have to add a (potentially in the future big) list of events. Any change of opinion based on this thought?

As for the line endings this is automatically enforced by my IDE according to the PEP8 rules. I'll see if I can override it.

@metachris
Copy link
Contributor

I still think it would be better to give the user immediate help instead of having them go through the help command.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 66.132% when pulling c496a2e on ixje:clarify_config_error into 86a62df on CityOfZion:development.

@localhuman localhuman merged commit 09a989b into CityOfZion:development Jan 6, 2018
@ixje ixje deleted the clarify_config_error branch January 22, 2018 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants