-
Notifications
You must be signed in to change notification settings - Fork 201
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
Update the configuration schema for consistency #267
Conversation
pylsp/config/schema.json
Outdated
@@ -281,7 +281,7 @@ | |||
"pylsp.plugins.pydocstyle.convention": { | |||
"type": ["string", "null"], | |||
"default": null, | |||
"enum": ["pep257", "numpy", null], | |||
"enum": ["pep257", "numpy", "None"], |
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.
Note, I changed the enum
array value here to include "None"
instead of null
as "None"
was listed as an allowed value in CONFIGURATION.md. But perhaps it should be ["pep257", "numpy", "None", null]
instead?
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'd say keep the null as otherwise default will not validate
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.
If I make it ["pep257", "numpy", "None", null]
then regenerating the CONFIGURATION.md file results in:
string
(one of:pep257
,numpy
,None
,None
)
Is that an error in the script?
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.
Yes, we should probably distinguish between null and None better in the conversion script.
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.
keep the null as otherwise default will not validate
Not sure I understand (be aware I'm only just learning to programme). Where does the default value get validated and wouldn't its value simply be taken as the associated value of the "default"
key which is null
in this case anyway?
The value associated with the "enum"
key is a list (to python) but I don't understand how null
can be a valid list item since it is not a string. I understand it is a valid item within a JSON array.
I don't understand how the following line takes the item null
from the list and adds None
to the allowed_values
list created with the list comprehension.
allowed_values = [f"`{value}`" for value in prop["enum"]] |
Incidentally, shouldn't this line include quotes around the allowed string values as the user must set a string value?
allowed_values = [f"`\"{value}\"`" for value in prop["enum"]]`
Thanks for your patience.
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 understand how the following line takes the item null from the list and adds None to the allowed_values list created with the list comprehension.
It seems Python when dealing with a JSON array interprets null
as None
but otherwise would raise an exception.
print(prop["enum"]) # ['pep25', 'numpy', 'None', None]
Another misunderstanding I have is why null
is included in this "enum" array but not in that for pylsp.configurationSources
. Is it because in that case the "default"
value is not null
?
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.
Any further thoughts on this? If not I think I will revert it back to how it was but add the quotes in the allowed_values in L44.
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.
Yes, please revert this one.
dding strings for clarity sounds ok, but the implementation should use repr instead (via !r
formatter) as it will handle more edge cases like escaping and properly denote allowed enum values which are not strings:
allowed_values = [f"`{value!r}`" for value in prop["enum"]]
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.
OK done and thanks I learned about !r
today. Rebased onto latest develop
branch. Hope I did so correctly. Thanks again for your time.
Squashed my three commits for a cleaner history. |
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.
LGTM, thanks!
null
configuration option value.Resolves #263
Resolves #264.