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

Validate pytest-mypy-plugins input file schema #127

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

stdedos
Copy link
Contributor

@stdedos stdedos commented Sep 6, 2023

Create a schema.yaml to:

  • Validate the input provided by the users
  • Offer in-editor validation and auto-completion
  • Easily keep the documentation of it up-to-date

Use said schema to meta-test all test files for conformance.

Additionally:

  • Fix mypy_config type to str | None
  • Update jinja2.defaults.VARIABLE_START_STRING to the more-correct _rendering_env.variable_start_string.
  • Update .gitignore

This fixes the real issue behind #124: The problem was not that mypy_config MUST HAVE {{ when parametrized was set; It was passing a list (of dicts) - which that was not templatable.

Signed-off-by: Stavros Ntentos 133706+stdedos@users.noreply.github.com

@stdedos stdedos force-pushed the impr/input-schema branch 3 times, most recently from 8f3dc1c to ac26430 Compare September 6, 2023 08:41
@stdedos
Copy link
Contributor Author

stdedos commented Sep 6, 2023

More details about the changes:

The "main" schema is written in schema.yaml. The schema.json and schema.md are supportive files:

The files can be generated, from the repo root:

(
	set -e
	cd pytest_mypy_plugins
	yq -o=json schema.yaml >| schema.json
	jsonschema2md schema.json schema.md --examples-as-yaml
)

I haven't seen any kind of "repo/release automation" setup, so I have avoided adding those commands. Also, two external tools are required for this, so ... some extra thinking might be needed. I'd recommend pre-commit and custom hooks - but, up to you.

After that, you can do this:

image

... or, if you'd like more ✨ magic ✨ , then you can deploy it to SchemaStore (https://github.com/SchemaStore/schemastore/blob/2991ef67756c3bacfaa1bcb490c08922c13ca3f1/src/api/json/catalog.json#L1835-L1840) - but you'd have to "force-rename" the yml files to be picked-up automatically (e.g.,test_*.mypy.yaml?)

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks! I generally support this feature, the only thing that bothers me is that we have 3 different schema files :)

Can we just use schema.json for now? Or yaml one?

pytest_mypy_plugins/collect.py Show resolved Hide resolved
@stdedos
Copy link
Contributor Author

stdedos commented Sep 8, 2023

... the only thing that bothers me is that we have 3 different schema files :)

Well ... we really have one. schema.yaml.

Can we just use schema.json for now?

Well ... yes 😢
But it will make me SO MUCH MORE SAD writing it 😭

Or yaml one?

The yaml file "cannot be used" with anything else. It's called JSONSchema 😉

@stdedos stdedos force-pushed the impr/input-schema branch 2 times, most recently from a9496b6 to 785fa6b Compare September 8, 2023 08:18
@sobolevn
Copy link
Member

sobolevn commented Sep 8, 2023

I think that yaml is a superset of json. So, can't we just load yaml file and convert it to json in python?

This way we can at least try to have a single file for IDE-schema and validation.

@stdedos
Copy link
Contributor Author

stdedos commented Sep 8, 2023

I think that yaml is a superset of json. So, can't we just load yaml file and convert it to json in python?

You can do that for https://github.com/typeddjango/pytest-mypy-plugins' validation. "IDE schema.json readers" fetch a JSON-file.

I could've, instead, avoided the load here https://github.com/typeddjango/pytest-mypy-plugins/pull/127/files#diff-f3f061be87eac60afb6957b8b8ea08b948ca1a2b12b7584f99c7611adba6232aR33 and just give "directly" the JSON file.

But then you'd have "two dependencies": yaml being the nice prototype (which you are not shipping anywhere), and the json file (which you need to remember to "change it" every time you do changes on yaml file - which, right now, they are not formally verified to match)

@sobolevn
Copy link
Member

sobolevn commented Sep 8, 2023

@stdedos yes, I think that this is good thing to start with: just one json file :)
Later we can add more if needed.

@stdedos stdedos force-pushed the impr/input-schema branch 2 times, most recently from c1a3a00 to f37b562 Compare September 8, 2023 12:12
Create a `schema.json` to:
* Validate the input provided by the users
* Offer in-editor validation and auto-completion
* Easily keep the documentation of it up-to-date

Use said schema to meta-test all test files for conformance.

Additionally:
* Fix `mypy_config` type to `str | None`
* Update `jinja2.defaults.VARIABLE_START_STRING` to the
  more-correct `_rendering_env.variable_start_string`.
* Update `.gitignore`

This fixes the real issue behind typeddjango#124:
The problem was not that `mypy_config` *MUST HAVE* `{{` when `parametrized` was set;
It was passing a `list` (of `dict`s) - which that was not templatable.

Signed-off-by: Stavros Ntentos <133706+stdedos@users.noreply.github.com>
@stdedos
Copy link
Contributor Author

stdedos commented Sep 8, 2023

Ok 🤷

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍
Very impressive work.

Comment on lines +19 to +21
{
"case": "999"
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
"case": "999"
},
{
"case": "999"
},

Copy link
Contributor Author

@stdedos stdedos Sep 8, 2023

Choose a reason for hiding this comment

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

??

image

@sobolevn sobolevn merged commit e7e1a0f into typeddjango:master Sep 8, 2023
9 checks passed
@sobolevn
Copy link
Member

sobolevn commented Sep 8, 2023

GitHub does not understand this for some reason :)

@stdedos
Copy link
Contributor Author

stdedos commented Sep 8, 2023

If you start the

if you'd like more ✨ magic ✨ , then you can deploy it to SchemaStore (SchemaStore/schemastore@2991ef6/src/api/json/catalog.json#L1835-L1840) - but you'd have to "force-rename" the yml files to be picked-up automatically (e.g.,test_*.mypy.yaml?)

process, lemme know (and/or add it on your README.md)

@sobolevn
Copy link
Member

sobolevn commented Sep 8, 2023

I am not sure that I understand what that is :)

@stdedos
Copy link
Contributor Author

stdedos commented Sep 8, 2023

You can ask https://github.com/SchemaStore/schemastore/ to include your schema in their database, so that editors pick it up automagically.

However, that means you'd have to suggest to people to name their files "more specifically", so that a glob pattern configures it automatically. They can always manually type the comment above, if they don't want to rename their tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants