-
Notifications
You must be signed in to change notification settings - Fork 77
🔧 Make jsonschema dependency optional #1540
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: sigma67 <sigma67.github@gmail.com>
Signed-off-by: sigma67 <sigma67.github@gmail.com>
b247208
to
9bbdcb7
Compare
Signed-off-by: sigma67 <sigma67.github@gmail.com>
idk about the docker tests, seems to be some kind of unrelated auth problem
|
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.
Thanks for the PR, I have some feedback.
[project.optional-dependencies] | ||
plotting = ["matplotlib>=3.3.0"] # for needpie / needbar | ||
schema = [ | ||
"jsonschema[format]>=3.2.0", |
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 split this, e.g. into groups schema
and schema-formats
. If jsonschema
is available, all validations except the string patterns are possible. jsonschema
might already be installed in some environments by other libraries.
If schema-formats
is not installed, it would remove these dependencies:
+ arrow==1.3.0
+ fqdn==1.5.1
+ isoduration==20.11.0
+ jsonpointer==3.0.0
+ rfc3339-validator==0.1.4
+ rfc3987==1.3.8
+ types-python-dateutil==2.9.0.20250822
+ uri-template==1.3.0
+ webcolors==24.11.1
jsonschema
itself only requires:
+ attrs==25.4.0
+ jsonschema==4.25.1
+ jsonschema-specifications==2025.9.1
+ referencing==0.36.2
+ rpds-py==0.27.1
Edit: See below, I propose to only have schema-formats
and leave jsonschema
as a required dependency.
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 yeah I agree, most of the trouble comes from the [format] extra
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.
Could you clarify what changes are required as a result of this? How can we skip only the string patterns during validation?
pip install sphinx-needs | ||
To use schema validation features of sphinx-needs, you need to also install ``jsonschema``, which is available *via* the ``schema`` extra: |
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.
To use schema validation features of sphinx-needs, you need to also install ``jsonschema``, which is available *via* the ``schema`` extra: | |
To use schema validation features of sphinx-needs, you need to also install ``jsonschema``, which is available via the ``schema`` extra: |
@@ -0,0 +1,13 @@ | |||
"""These tests should only be run in an environment without sphinx_needs installed.""" |
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.
Not sure how this test relates to what you are implementing. sphinx_needs
is certainly installed :)
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.
see the ci.yaml changes, this follows the same structure as the no_matplotlib tests.
The relevant ci job uninstalls the dependency jsonschema and runs only this file
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 would expect a positive test that checks whether the build without formats still work.
Then a negative test that checks whether the warning is triggered that certain packages are missing.
indirect=True, | ||
) | ||
def test_needsfile(test_app): | ||
"""Test the build fails correctly, if matplotlib is not installed.""" |
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.
Typo?
|
||
@lru_cache | ||
def import_jsonschema() -> ModuleType | None: | ||
"""Import and return matplotlib, or return None if it cannot be imported. |
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.
"""Import and return matplotlib, or return None if it cannot be imported. | |
"""Import and return jsonschema, or return None if it cannot be imported. |
def import_jsonschema() -> ModuleType | None: | ||
"""Import and return matplotlib, or return None if it cannot be imported. | ||
Also sets the interactive backend to ``Agg``, if ``DISPLAY`` is not 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.
Copy paste error.
dependencies = [ | ||
"sphinx>=7.4,<9", | ||
"requests-file~=2.1", # external links | ||
"requests~=2.32", # external links |
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.
jsonschema
was a dependency before for version 5.1.0
as we see in this PR as it was used to check the schema of imported needs.json files. I think that will still be required. This is a basic functionality. So I would only add a group schema-formats
that only cares for the additional formats dependencies.
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.
valid, will fix this
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.
One more note, the check whether schema-formats
is required or not should be done by looking at needs_extra_options.schema and the schema definition rules early during config loading. For needs_extra_options that is fairly easy, for the schemas it's some tree walking as in def resolve_refs()
or def populate_field_type()
.
That sounds a bit beyond my experience with this project. Feel free to take over that part if you have time |
closes #1525