-
Notifications
You must be signed in to change notification settings - Fork 153
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
Added new FormElements to represent floats and string input text boxes #653
Conversation
from ..custom_viewer import FormElement, NumberElement, \ | ||
ChoiceElement, CustomViewer, \ | ||
CustomSubsetState, AttributeInfo, \ | ||
FloatElement, TextBoxElement |
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.
nit: prefer using parentheses instead of backslashes
Nice -- can you add a few screenshots to this PR? What is the specific error message you get? For state saving to work you need to implement the state property (which it looks like you attempt to do via the |
@classmethod | ||
def recognizes(cls, params): | ||
try: | ||
if params.startswith('float'): |
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.
We should think about how exactly float and text widgets should be recognized -- what you have might be fine, but it's an important decision. Should we be considering other alternatives to 'float()' and 'str()'?
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.
Yeah, I agree these may not be the best choices. I'm open to suggestions. I was thinking it would also be useful to recognize just floats ... something akin to a=2.7
.
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.
Yeah that might be a better option to start with (assuming it doesn't collide with any other FormElement -- I need to check). Likewise the text element could recognize strings (again, in a way that doesn't break the behavior of special strings like 'att')
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.
While I know the idea of the custom_viewers is to make things easier for non-developers; have you thought about allowing the settings definitions to recognize form elements directly? Something like:
cv = custom_viewer('CV', a=NumberElement(1, 10))
We could support both pretty easily.
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.
That's a good idea (I would probably docuement that that in the docstring, but perhaps not sphinx documentation)
@ChrisBeaumont , I'm putting together a medium-sized plugin using a subclass of the |
If it's equally convenient, 2 PRs is probably easier to review! |
Done, let me spend a little time cleaning up and improving some test coverage and then I'll call this one ready. |
093eede
to
b891fae
Compare
@JudoWill - there is a real failure here with Python 2.6 due to the fact that 2.6 doesn't support the |
Fixed py26 issue. Ready to merge. |
Looks good to me! Thanks @JudoWill |
👍 |
Added new FormElements to represent floats and string input text boxes
Some commits related to issues #650. @ChrisBeaumont , I'm getting a weird error related to saving the custom viewer, can you take a look and point me in the right direction.
Some doc changes are coming in the next push.