Skip to content

Conversation

sigma67
Copy link

@sigma67 sigma67 commented Oct 7, 2025

closes #1525

Signed-off-by: sigma67 <sigma67.github@gmail.com>
Signed-off-by: sigma67 <sigma67.github@gmail.com>
Signed-off-by: sigma67 <sigma67.github@gmail.com>
@sigma67 sigma67 force-pushed the optional-jsonschema branch from b247208 to 9bbdcb7 Compare October 7, 2025 06:28
Signed-off-by: sigma67 <sigma67.github@gmail.com>
@sigma67
Copy link
Author

sigma67 commented Oct 7, 2025

idk about the docker tests, seems to be some kind of unrelated auth problem

Error: Username and password required

Copy link
Member

@ubmarco ubmarco left a 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",
Copy link
Member

@ubmarco ubmarco Oct 7, 2025

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.

Copy link
Author

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

Copy link
Author

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:
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
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."""
Copy link
Member

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 :)

Copy link
Author

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

Copy link
Member

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."""
Copy link
Member

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.
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
"""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.
Copy link
Member

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
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

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

valid, will fix this

@ubmarco ubmarco changed the title make jsonschema dependency optional 🔧 Make jsonschema dependency optional Oct 7, 2025
Copy link
Member

@ubmarco ubmarco left a 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().

@sigma67
Copy link
Author

sigma67 commented Oct 8, 2025

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

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 this pull request may close these issues.

sphinx-needs 6.0 adds too many additional dependencies

2 participants