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

Add support for native enum docstring #3668

Closed
wants to merge 2 commits into from

Conversation

jirikuncar
Copy link

@jirikuncar jirikuncar commented Oct 11, 2024

Apologies

I wasn't thorough in my research before creating this PR. I wanted to simplify the codebase by removing many enum decorators and moving the description to docstrings where I would expect it. As I appreciate maintainers time, I would understand if this PR gets closed without any comment. If you find this PR useful please continue in the discussion #653.

Description

Adds support for native enum __doc__.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

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). 🤞

Summary by Sourcery

Introduce support for using native enum docstrings as descriptions in schema enum types, enhance the enum creation process to incorporate these docstrings, and add corresponding tests and documentation.

New Features:

  • Add support for using native enum __doc__ as the description for schema enum types.

Enhancements:

  • Enhance the create_enum function to utilize the native enum __doc__ for descriptions.

Documentation:

  • Add a RELEASE.md file documenting the use of native enum __doc__ for schema descriptions.

Tests:

  • Add a test to verify that the native enum __doc__ is correctly used as the description in the schema.

Copy link
Contributor

sourcery-ai bot commented Oct 11, 2024

Reviewer's Guide by Sourcery

This pull request adds support for native enum docstrings in Strawberry. The implementation focuses on using the __doc__ attribute of enum classes as the description for the corresponding GraphQL enum type in the schema. This enhancement improves the automatic documentation of GraphQL schemas generated with Strawberry.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Add support for using native enum docstrings as GraphQL enum descriptions
  • Modify the create_enum method to use the enum's __doc__ attribute as the description
  • Update the strawberry_enum function call to include the description parameter
strawberry/annotation.py
Add test case for enum docstring support
  • Create a new test function test_default_enum_with_docstring
  • Define a sample enum class with a docstring
  • Assert that the enum's docstring is used as the description for the GraphQL type
tests/enums/test_enum.py
Document the new feature in the release notes
  • Create a new RELEASE.md file
  • Describe the new feature and its impact on the generated GraphQL schema
  • Provide a code example demonstrating the usage of enum docstrings
RELEASE.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@@ -0,0 +1,24 @@
Release type: patch
Copy link
Author

Choose a reason for hiding this comment

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

not sure here

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @jirikuncar - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider updating the project documentation to mention this new feature of using native enum docstrings as descriptions in the schema. This will help users discover and utilize this enhancement.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 1 issue found

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 and I'll use the feedback to improve your reviews.

Comment on lines +191 to +192
evaled_type, description=evaled_type.__doc__
)._enum_definition
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider handling cases where doc might be None

The current implementation assumes that evaled_type.doc always exists. Consider using a default value or a conditional to handle cases where the docstring might be None, e.g., description=evaled_type.__doc__ or ''.

Suggested change
evaled_type, description=evaled_type.__doc__
)._enum_definition
evaled_type,
description=evaled_type.__doc__ if evaled_type.__doc__ is not None else ''
)._enum_definition

Copy link
Author

Choose a reason for hiding this comment

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

description can be None 🤔

RELEASE.md Show resolved Hide resolved
@botberry
Copy link
Member

botberry commented Oct 11, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Use native enum __doc__ as the description for the schema enum type.

This change improves consistency between Python enums and schema representations,
and leverages existing documentation for better maintainability and clarity.

# somewhere.py
from enum import Enum


class AnimalKind(Enum):
    """The kind of animal."""

    AXOLOTL, CAPYBARA = range(2)

This will generate the following schema:

"""The kind of animal."""
enum AnimalKind {
  AXOLOTL
  CAPYBARA
}

Here's the tweet text:

🆕 Release (next) is out! Thanks to @jirikuncar for the PR 👏

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

Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@enoua5
Copy link
Contributor

enoua5 commented Oct 11, 2024

There's been previous discussion about using docstrings to generate descriptions (see #653). The general consensus has been that people are worried about unintentionally leaking internal comments because they weren't expecting this behaviour. That being said, with the native enums, we don't have the @strawberry.enum decorator to add the description field on. Maybe including an opt-in configuration option about it would work? I'm honestly not 100% sure.

If we are adding this feature, I think it would also be good to allow docstrings to set the descriptions for the enum values as well (we currently have FOO = strawberry.enum_value('FOO', description="Description of foo")). I can imagine something like this generating descriptions for BAR and BAZ:

class Foo(Enum):
    """This is a Foo enum."""

    BAR = "bar"
    """This is bar"""

    BAZ = "baz"
    """This is baz"""

@jirikuncar
Copy link
Author

  1. If the native enum support was accepted then we generally acknowledged that users know the consequences. I would get the argument of leaking implementation details for any other class but enums.

  2. I am not aware how to easily access variable docstring without ast. The alternative could be using type annotations, but not sure if that's something people would like to see in this PR.

@nrbnlulu
Copy link
Member

I think this should be configurable at schema level. so when we pass stuff to graphql-core we use this condition and iterate over all the types, if there is no doc we take the doc from the origin class.

@jirikuncar jirikuncar closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants