Skip to content
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

Errors in CONFIGURATION.md? #264

Closed
doolio opened this issue Sep 12, 2022 · 6 comments · Fixed by #267
Closed

Errors in CONFIGURATION.md? #264

doolio opened this issue Sep 12, 2022 · 6 comments · Fixed by #267
Milestone

Comments

@doolio
Copy link
Contributor

doolio commented Sep 12, 2022

pylsp.plugins.flake8.hangClosing is of type boolean but the default value is null. Should it not be true or false?
pylsp.plugins.flake8.maxLineLength is of type integer but should it not be number (c.f pylsp.plugins.pycodestyle.maxLineLength) or does it not matter?
pylsp.plugins.flake8.indentSize is of type integer but should it not be number or does it not matter?
pylsp.plugins.flake8.select is of type string array but the default value is null. Should it not be [] (c.f pylsp.plugins.pydocstyle.select?
pylsp.plugins.jedi.env_vars is of type object but the default value is null. Should it not be {}?
pylsp.plugins.pycodestyle.hangClosing is of type boolean but the default value is null. Should it not be true or false?
pylsp.plugins.pycodestyle.indentSize is of type integer but should it not be number or does it not matter?
pylsp.plugins.pydocstyle.convention is of type string but the default value is null yet the allowed values are "None", "numpy" or "pep257". Should it not be "None" instead of null.
pylsp.rope.ropeFolder is of type string array but the default value is null. Should it not be []?

See also #263.

@krassowski
Copy link
Contributor

Thanks for taking a close look. I did not do not anything wrong here, but we could improve CONFIGURATION.md by explaining what null means in this context and make pylsp.plugins.pycodestyle.maxLineLength and integer:

  • pylsp.plugins.flake8.hangClosing, pylsp.plugins.flake8.select, pylsp.plugins.pycodestyle.hangClosing, pylsp.plugins.pydocstyle.convention, pylsp.rope.ropeFolder: Generally for third party tools like flake8, null means that we do not set the value and therefore use the tool's default; the default might change between versions of flake8, pycodestyle and may be different based on configuration sources and environment of the user. Therefore null is different from None. This could be explicitly mentioned in CONFIGURATION.md - would you like to contribute?
  • pylsp.plugins.flake8.maxLineLength: integer is correct; how could a line be 80.5 character?
  • pylsp.plugins.pycodestyle.maxLineLength as per above should be changed to integer
  • pylsp.plugins.flake8.indentSize and pylsp.plugins.pycodestyle.indentSize again, can you enter three and a half spaces? I am pretty sure integer is correct; someone could even restrict it to positive integer but it is more difficult to express in underlying schema

@doolio
Copy link
Contributor Author

doolio commented Sep 13, 2022

Generally for third party tools like flake8, null means that we do not set the value and therefore use the tool's default; the default might change between versions of flake8, pycodestyle and may be different based on configuration sources and environment of the user. Therefore null is different from None. This could be explicitly mentioned in CONFIGURATION.md - would you like to contribute?

I see. That make sense. I'd be happy to update the configuration file but I do so by updating pylsp/config/schema.json, right?

integer is correct; how could a line be 80.5 character?
again, can you enter three and a half spaces?

Agreed. However, I had assumed it should be number as I thought that the JSON datatype was number to cover both integer and floating point number types. Is that not the case? I'm only starting to learn how to programme (starting with python!) so eager to learn.

@krassowski
Copy link
Contributor

I'd be happy to update the configuration file but I do so by updating pylsp/config/schema.json, right?

Yes, that is preferred as it will then be in both schema.json and CONFIGURATION.md (after regenerating it). You could update the description field to add it above the table:

"description": "This server can be configured using `workspace/didChangeConfiguration` method. Each configuration option is described below:",

Alternatively it could be added below the table. We implement it by by adding a comment field in schema.json like "$comment": "extra description for our use only" and then adding it below the table in convert_schema function:

def convert_schema(schema: dict, source: str = None) -> str:
lines = [
f"# {schema['title']}",
schema["description"],
"",
"| **Configuration Key** | **Type** | **Description** | **Default** ",
"|----|----|----|----|",
]
for key, prop in schema["properties"].items():
description = prop.get("description", "")
default = json.dumps(prop.get("default", ""))
lines.append(
f"| `{key}` | {describe_type(prop)} | {description} | `{default}` |"
)
if source:
lines.append(
f"\nThis documentation was generated from `{source}`."
" Please do not edit this file directly."
)
# ensure empty line at the end
lines.append("")
return "\n".join(lines)

I had assumed it should be number as I thought that the JSON datatype was number to cover both integer and floating point number types. Is that not the case?

JSON schema can encode much more than the JSON data types can hold. "Understanding JSON Schema" is probably a good place to start https://json-schema.org/understanding-json-schema/reference/numeric.html#integer

@doolio
Copy link
Contributor Author

doolio commented Sep 13, 2022

Thanks for the link. It is clearer now.

Should pylsp.plugins.jedi_completion.resolve_at_most and pylsp.plugins.mccabe.threshold be of type integer too for consistency with the other integer datatypes in this table? And by the same argument should the default values of pylsp.plugins.pycodestyle.select and pylsp.plugins.pydocstyle.select be set to null for consistency with pylsp.plugins.flake8.select?

[Aside: Is there any plans to include other third party plugins settings in schema.json. I'm thinking of python-lsp-black, pyls-isort (which hopefully is renamed to python-lsp-isort) etc. so they too can be passed by the client to the server?]

Edit: Added another setting to a query.

@krassowski
Copy link
Contributor

Should pylsp.plugins.jedi_completion.resolve_at_most and pylsp.plugins.mccabe.threshold be of type integer too for consistency with the other integer datatypes in this table? And by the same argument should the default value of pylsp.plugins.pydocstyle.select be set to null for consistency with pylsp.plugins.flake8.select?

Yes, these suggestions sound good to me.

[Aside: Is there any plans to include other third party plugins settings in schema.json. I'm thinking of python-lsp-black, pyls-isort (which hopefully is renamed to python-lsp-isort) etc. so they too can be passed by the client to the server?]

I think we could encourage plugins which are in separate repositories to add their schema in their repositories; we could make re-uisng the script for converting schema to markdown file easier, but I don't think that we should add those to schema.json in this repository (due to high risk of user confusion, changes between versions, what happens if someone forks the plugin and now you want to use a fork, etc).

@doolio
Copy link
Contributor Author

doolio commented Sep 14, 2022

but I don't think that we should add those to schema.json in this repository (due to high risk of user confusion, changes between versions, what happens if someone forks the plugin and now you want to use a fork, etc).

OK, I understand. I did not appreciate such concerns.

@ccordoba12 ccordoba12 added this to the v1.6.0 milestone Sep 14, 2022
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 a pull request may close this issue.

3 participants