-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,6 @@ 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 commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. valid, will fix this |
||
"jsonschema[format]>=3.2.0", # schema validation for needsimport and ontology | ||
"sphinx-data-viewer~=0.1.5", # needservice debug output | ||
"sphinxcontrib-jquery~=4.0", # needed for datatables in sphinx>=6 | ||
"tomli; python_version < '3.11'", # for needs_from_toml configuration | ||
|
@@ -40,9 +39,13 @@ dependencies = [ | |
|
||
[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 commentThe reason will be displayed to describe this comment to others. Learn more. We should split this, e.g. into groups
Edit: See below, I propose to only have There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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? |
||
] # for schema validation for needsimport and ontology | ||
test = [ | ||
"defusedxml~=0.7.1", | ||
"matplotlib>=3.3.0", | ||
"jsonschema[format]>=3.2.0", | ||
"pytest~=8.0", | ||
"pytest-cov~=6.0", | ||
"syrupy~=4.0", | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,6 +8,7 @@ | |||||
from collections.abc import Callable | ||||||
from dataclasses import dataclass | ||||||
from functools import lru_cache, reduce, wraps | ||||||
from types import ModuleType | ||||||
from typing import TYPE_CHECKING, Any, Protocol, TypeVar | ||||||
from urllib.parse import urlparse | ||||||
|
||||||
|
@@ -419,6 +420,19 @@ def import_matplotlib() -> matplotlib | None: | |||||
return matplotlib | ||||||
|
||||||
|
||||||
@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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Copy paste error. |
||||||
""" | ||||||
try: | ||||||
import jsonschema | ||||||
except ImportError: | ||||||
return None | ||||||
return jsonschema | ||||||
|
||||||
|
||||||
def save_matplotlib_figure( | ||||||
app: Sphinx, figure: FigureBase, basename: str, fromdocname: str | ||||||
) -> nodes.image: | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how this test relates to what you are implementing. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
|
||
import pytest | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"test_app", | ||
[{"buildername": "html", "srcdir": "doc_test/doc_needsfile"}], | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Typo? |
||
test_app.build() |
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.