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

Fix unclear error when missing dependencies #3511

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions RELEASE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Release type: minor

When calling the CLI without all the necessary dependencies installed,
a `MissingOptionalDependenciesError` will be raised instead of a
`ModuleNotFoundError`. This new exception will provide a more helpful
hint regarding how to fix the problem.
20 changes: 12 additions & 8 deletions strawberry/cli/__init__.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
from .commands.codegen import codegen as codegen # noqa
from .commands.export_schema import export_schema as export_schema # noqa
from .commands.server import server as server # noqa
from .commands.upgrade import upgrade as upgrade # noqa
from .commands.schema_codegen import schema_codegen as schema_codegen # noqa
try:
from .app import app
from .commands.codegen import codegen as codegen # noqa
from .commands.export_schema import export_schema as export_schema # noqa
from .commands.schema_codegen import schema_codegen as schema_codegen # noqa
from .commands.server import server as server # noqa
from .commands.upgrade import upgrade as upgrade # noqa

from .app import app
def run() -> None:
app()

except ModuleNotFoundError as exc:
from strawberry.exceptions import MissingOptionalDependenciesError

Check warning on line 13 in strawberry/cli/__init__.py

View check run for this annotation

Codecov / codecov/patch

strawberry/cli/__init__.py#L12-L13

Added lines #L12 - L13 were not covered by tests

def run() -> None:
app()
raise MissingOptionalDependenciesError(extras=["cli"]) from exc

Check warning on line 15 in strawberry/cli/__init__.py

View check run for this annotation

Codecov / codecov/patch

strawberry/cli/__init__.py#L15

Added line #L15 was not covered by tests
parafoxia marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +1 to +15
Copy link
Member

Choose a reason for hiding this comment

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

that's great, it's something I was thinking about a few weeks ago!

Do you think it would be possible to make this granular too? Maybe it's not worth it, but some commands need different dependencies (for example schema codegen needs libcst, most of the other commands don't), so maybe we can suggest to install libcst or [cli] in that case?

But I don't know if we can do this cleanly 🤔

Copy link
Contributor Author

@parafoxia parafoxia May 24, 2024

Choose a reason for hiding this comment

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

Oh wow, how convenient!

If you did want to do that, the cleanest way would probably be to import anything necessary in the commands themselves and raise errors within that. That would add quite a few lines (unless you went to the trouble of using a decorator to transform the error or something), but would get extra granularity when needed.

Idk how worth it it would be though honestly, unless libcst installs like 20 dependencies of its own.

I did do it like that for a personal project of mine though, where you could use it without the dependencies, but would need them for extra functionality: https://github.com/parafoxia/analytix/blob/main/analytix/reports/interfaces.py#L315-L318

Copy link
Member

Choose a reason for hiding this comment

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

yeah, maybe it's not worth doing it for now

I want to add some more commands in future, maybe I can do that then 😊

I'll properly review this and merge later! thanks :)

2 changes: 2 additions & 0 deletions strawberry/exceptions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from .invalid_argument_type import InvalidArgumentTypeError
from .invalid_union_type import InvalidTypeForUnionMergeError, InvalidUnionTypeError
from .missing_arguments_annotations import MissingArgumentsAnnotationsError
from .missing_dependencies import MissingOptionalDependenciesError
from .missing_field_annotation import MissingFieldAnnotationError
from .missing_return_annotation import MissingReturnAnnotationError
from .not_a_strawberry_enum import NotAStrawberryEnumError
Expand Down Expand Up @@ -186,4 +187,5 @@ class StrawberryGraphQLError(GraphQLError):
"MissingFieldAnnotationError",
"DuplicatedTypeName",
"StrawberryGraphQLError",
"MissingOptionalDependenciesError",
]
20 changes: 20 additions & 0 deletions strawberry/exceptions/missing_dependencies.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
from __future__ import annotations

from typing import Optional


class MissingOptionalDependenciesError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

I've changed this to just be based on Exception 😊

"""Some optional dependencies that are required for a particular
task are missing."""

def __init__(
self,
*,
packages: Optional[list[str]] = None,
extras: Optional[list[str]] = None,
) -> None:
packages = packages or []
if extras:
packages.append(f"'strawberry-graphql[{','.join(extras)}]'")
hint = f" (hint: pip install {' '.join(packages)})" if packages else ""
self.message = f"Some optional dependencies are missing{hint}"
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import pytest
parafoxia marked this conversation as resolved.
Show resolved Hide resolved

from strawberry.exceptions import MissingOptionalDependenciesError


def test_missing_optional_dependencies_error():
parafoxia marked this conversation as resolved.
Show resolved Hide resolved
with pytest.raises(MissingOptionalDependenciesError) as exc_info:
raise MissingOptionalDependenciesError()

assert exc_info.value.message == "Some optional dependencies are missing"


def test_missing_optional_dependencies_error_packages():
parafoxia marked this conversation as resolved.
Show resolved Hide resolved
with pytest.raises(MissingOptionalDependenciesError) as exc_info:
raise MissingOptionalDependenciesError(packages=["a", "b"])

assert (
exc_info.value.message
== "Some optional dependencies are missing (hint: pip install a b)"
)


def test_missing_optional_dependencies_error_empty_packages():
with pytest.raises(MissingOptionalDependenciesError) as exc_info:
raise MissingOptionalDependenciesError(packages=[])

assert exc_info.value.message == "Some optional dependencies are missing"


def test_missing_optional_dependencies_error_extras():
parafoxia marked this conversation as resolved.
Show resolved Hide resolved
with pytest.raises(MissingOptionalDependenciesError) as exc_info:
raise MissingOptionalDependenciesError(extras=["dev", "test"])

assert (
exc_info.value.message
== "Some optional dependencies are missing (hint: pip install 'strawberry-graphql[dev,test]')"
)


def test_missing_optional_dependencies_error_empty_extras():
with pytest.raises(MissingOptionalDependenciesError) as exc_info:
raise MissingOptionalDependenciesError(extras=[])

assert exc_info.value.message == "Some optional dependencies are missing"


def test_missing_optional_dependencies_error_packages_and_extras():
with pytest.raises(MissingOptionalDependenciesError) as exc_info:
raise MissingOptionalDependenciesError(
packages=["a", "b"],
extras=["dev", "test"],
)

assert (
exc_info.value.message
== "Some optional dependencies are missing (hint: pip install a b 'strawberry-graphql[dev,test]')"
)
Loading