Skip to content

Conversation

@kmvanbrunt
Copy link
Member

Closes #870 #871

@codecov
Copy link

codecov bot commented Feb 5, 2020

Codecov Report

Merging #873 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
+ Coverage   97.37%   97.39%   +0.01%     
==========================================
  Files          14       14              
  Lines        3580     3602      +22     
==========================================
+ Hits         3486     3508      +22     
  Misses         94       94              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60a212c...c4893ea. Read the comment docs.

tleonhardt
tleonhardt previously approved these changes Feb 6, 2020
Copy link
Member

@tleonhardt tleonhardt left a comment

Choose a reason for hiding this comment

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

Looks good

@kotfu
Copy link
Member

kotfu commented Feb 6, 2020

This all looks pretty good to me. I'll add one suggestion.

If the list of valid values is not known at initialization time (which is when settables would typically get defined), you have to do really awkward stuff in order to validate the new value of a setting when a user changes it. The onchange_cb doesn't have access to self (i.e. the cmd2 subclass), so my callback doesn't have access to all the data in my app.

If I somehow manage to say the input isn't valid, there is no way for my callback to to say "hey this isn't a valid value, I don't want to allow the user to change it" back to cmd2. I think throwing a ValueError exception would be ideal here, and cmd2 could then catch it and not allow the change in the value.

@kmvanbrunt
Copy link
Member Author

kmvanbrunt commented Feb 6, 2020

@kotfu The onchange_cb can have access to self if you set it to a bound method of your app.

onchange_cb = self.val_changed

As far as validation, that's the job of Settable.val_type. It corresponds to the type field in the argparse.add_argument() function. Any exception it raises is handled in do_set() by printing it with perror() and not changing the value.

@kotfu
Copy link
Member

kotfu commented Feb 6, 2020

Go it. I missed that val_type was a callable. You already have implemented my suggestion. This looks good to me.

@kmvanbrunt
Copy link
Member Author

@kotfu I update the docstring for Settable to be more clear.

@kotfu
Copy link
Member

kotfu commented Feb 6, 2020

hold up before we merge this, I need to fix the settings.rst documentation and update examples/environment.py

tleonhardt
tleonhardt previously approved these changes Feb 6, 2020
@tleonhardt
Copy link
Member

This looks good so I'm going to merge it

@tleonhardt tleonhardt merged commit c7ac2e9 into master Feb 6, 2020
@tleonhardt tleonhardt deleted the set_update branch February 6, 2020 23:20
@tleonhardt tleonhardt linked an issue Feb 6, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tab complete settable attribute values setting settable attribute with default type None does not change

4 participants