-
Notifications
You must be signed in to change notification settings - Fork 13
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 support for Marker descriptions #1
Added support for Marker descriptions #1
Conversation
voluptuous_serialize/__init__.py
Outdated
else: | ||
pkey = key | ||
|
||
pval = convert(value) | ||
pval['name'] = pkey | ||
if description is not None: | ||
pval['description'] = str(description) |
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 not do this. There is no predefined type of the description field and so we can't force one.
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.
Would you be okay with passing the description
on as given?
fa0ce01
to
b2ccaf6
Compare
voluptuous_serialize/__init__.py
Outdated
if isinstance(key, vol.Marker): | ||
pkey = key.schema | ||
try: | ||
description = key.description | ||
except AttributeError: |
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.
When would this raise an AttributeError
?
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.
And if that is possible, let's add a test for it.
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 don't think it's possible, as you can see from my PR alecthomas/voluptuous#307): it's always set
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.
It's there because people may use an older version of voluptuous which doesn't yet have support for descriptions. I'm happy to add a test for it to ensure it can be handled.
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.
Actually, the current test case would also fail in 0.10.5 or older. Would you want to make this project to be 0.11.1+ only or should it support older versions as well?
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.
Oh, good point. I made this lib for using it in Home Assistant, so let's see if I can update Home Assistant to use 0.11 😉
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.
It appears you were able to upgrade to voluptuous 0.11.1? Any thoughts on whether you'd want this project to be 0.11.1+ only or to also support some older versions?
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, was able to upgrade. Let's just stick with 0.11.1+.
b2ccaf6
to
4ed9c27
Compare
Thanks, I have updated the commit to always assume |
awesome! are you planning on using this? |
If you are, I have a webcomponent here that can render these forms (or some parts of it). I also plan on adding a hook to allow people to define serialization strategies for custom validators. |
Yeah, I'll need to upgrade to voluptuous 0.11.1 as well first. I do have a use for this project as in one project I'm now maintaining both the schema and a separate description of that schema. Some functionality is still missing though, such as support for |
Cool! I literally just added enough for the needs of Home Assistant. |
@balloob this PR has not been released to PyPi. We could use it to enhance our login form right away. |
This adds support for
Marker
descriptions released yesterday in voluptuous 0.11.1 (alecthomas/voluptuous#307).I'm callling
str()
on the description because people may use text-like objects for their descriptions in their schemas (probably rare).