Fix unclear error when missing dependencies #3511

parafoxia · 2024-05-24T11:20:29Z

Description

This PR adds a new MissingOptionalDependenciesError and puts it to work in the cli module.

This was motivated by the previous ModuleNotFoundError causing confusion as to the exact cause of the error (especially as this would become a chain of six errors for the CLI). The new error emits a more helpful message, telling the user what they need to install to resolve the issue. It also makes it clearer that there are optional dependencies at work, especially in the case a user installs strawberry expecting the CLI to work out of the box.

The exception class is only in use in the CLI here as I'm not familiar enough with the rest of the code to implement it there (though if maintainers have any shortcuts for working out where it needs to be, happy to do that). The exception class is also futureproofed to hell (arguably overly so lmao) to allow seamless introduction wherever else seems fitting.

It's also tested, despite there not being a precedent to test exception classes. I took a punt at a best standard for this, but if maintainers want this removed or changed let me know.

I've also set this to minor instead of patch as it could cause breaking changes for a few users if they've gone about handling this themselves.

Types of Changes

Issues Fixed or Closed by This PR

None.

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

botberry · 2024-05-24T11:21:43Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

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.

Here's the tweet text:

🆕 Release (next) is out! Thanks to Ethan Henderson for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

sourcery-ai

Hey @parafoxia - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 4 issues found
🟢 Security: all looks good
🟡 Testing: 5 issues found
🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

strawberry/cli/__init__.py

strawberry/exceptions/missing_dependencies.py

tests/exceptions/classes/test_exception_class_missing_optional_dependencies_error.py

patrick91 · 2024-05-24T12:00:18Z

strawberry/cli/__init__.py

+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

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


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 🤔

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

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

patrick91

Looks 🔥 Thank you so much 😊

codecov · 2024-05-25T08:59:50Z

Codecov Report

Attention: Patch coverage is 93.75000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 96.52%. Comparing base (61f9e3a) to head (3121c07).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3511      +/-   ##
==========================================
- Coverage   96.53%   96.52%   -0.02%     
==========================================
  Files         516      518       +2     
  Lines       33174    33214      +40     
  Branches     5521     5522       +1     
==========================================
+ Hits        32025    32060      +35     
- Misses        916      921       +5     
  Partials      233      233

codspeed-hq · 2024-05-25T09:02:38Z

CodSpeed Performance Report

Merging #3511 will not alter performance

_{Comparing parafoxia:fix-unclear-error-when-missing-dependencies (3121c07) with main (61f9e3a)}

Summary

✅ 12 untouched benchmarks

patrick91 · 2024-05-25T09:04:15Z

strawberry/exceptions/missing_dependencies.py

+from typing import Optional
+
+
+class MissingOptionalDependenciesError(Exception):


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

botberry · 2024-05-25T09:09:24Z

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

botberry added bot:has-release-file bot:release-type-minor labels May 24, 2024

sourcery-ai bot reviewed May 24, 2024

View reviewed changes

parafoxia added 4 commits May 24, 2024 12:26

Implement MissingOptionalDependenciesError

51669bd

Add testing for MissingOptionalDependenciesError

bd70a4a

Raise clearer error when the CLI extra dependencies need to be installed

edd5512

Add RELEASE.md for PR

c22e8f1

parafoxia force-pushed the fix-unclear-error-when-missing-dependencies branch from 0604f81 to c22e8f1 Compare May 24, 2024 11:26

Make suggested testing improvements

274f759

patrick91 reviewed May 24, 2024

View reviewed changes

patrick91 approved these changes May 25, 2024

View reviewed changes

Update base class

3121c07

patrick91 reviewed May 25, 2024

View reviewed changes

patrick91 merged commit dd4c343 into strawberry-graphql:main May 25, 2024
62 of 64 checks passed

parafoxia deleted the fix-unclear-error-when-missing-dependencies branch May 25, 2024 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unclear error when missing dependencies #3511

Fix unclear error when missing dependencies #3511

parafoxia commented May 24, 2024 •

edited

Loading

botberry commented May 24, 2024 •

edited

Loading

sourcery-ai bot left a comment

patrick91 May 24, 2024

parafoxia May 24, 2024 •

edited

Loading

patrick91 May 24, 2024

patrick91 left a comment

codecov bot commented May 25, 2024 •

edited

Loading

codspeed-hq bot commented May 25, 2024 •

edited

Loading

patrick91 May 25, 2024

botberry commented May 25, 2024

		from typing import Optional


		class MissingOptionalDependenciesError(Exception):

Fix unclear error when missing dependencies #3511

Fix unclear error when missing dependencies #3511

Conversation

parafoxia commented May 24, 2024 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

botberry commented May 24, 2024 • edited Loading

sourcery-ai bot left a comment

Choose a reason for hiding this comment

patrick91 May 24, 2024

Choose a reason for hiding this comment

parafoxia May 24, 2024 • edited Loading

Choose a reason for hiding this comment

patrick91 May 24, 2024

Choose a reason for hiding this comment

patrick91 left a comment

Choose a reason for hiding this comment

codecov bot commented May 25, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented May 25, 2024 • edited Loading

CodSpeed Performance Report

Merging #3511 will not alter performance

Summary

patrick91 May 25, 2024

Choose a reason for hiding this comment

botberry commented May 25, 2024

parafoxia commented May 24, 2024 •

edited

Loading

botberry commented May 24, 2024 •

edited

Loading

parafoxia May 24, 2024 •

edited

Loading

codecov bot commented May 25, 2024 •

edited

Loading

codspeed-hq bot commented May 25, 2024 •

edited

Loading