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 Enums in API docs #15740

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Conversation

davelopez
Copy link
Contributor

Apparently, string enums are converted to string differently between python versions making the make update-client-api-schema command return different results.

This should make sure the underlying value of the enum is printed instead. I've included python 3.11 to the OpenAPI linting GitHub Workflow to make sure it works.

How to test the changes?

  • This is a refactoring of components with existing test coverage.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Apparently there is a difference between versions of Python that will print those enums either as `SharingOptions.make_public` or `make_public`. This should print a consistent value.
@github-actions github-actions bot added this to the 23.1 milestone Mar 8, 2023
@nsoranzo
Copy link
Member

nsoranzo commented Mar 8, 2023

Can you make them typing_extensions.Literal instead of (str, Enum) please?
E.g.:

diff --git a/lib/galaxy/schema/schema.py b/lib/galaxy/schema/schema.py
index e462c3a7bc..d35cc8dc1d 100644
--- a/lib/galaxy/schema/schema.py
+++ b/lib/galaxy/schema/schema.py
@@ -2593,8 +2593,7 @@ class LibraryPermissionsPayload(LibraryPermissionsPayloadBase):
 # Library Folders -----------------------------------------------------------------
 
 
-class LibraryFolderPermissionAction(str, Enum):
-    set_permissions = "set_permissions"
+LibraryFolderPermissionAction = Literal["set_permissions"]
 
 
 FolderNameField: str = Field(
@@ -2975,12 +2974,8 @@ class HistoryContentsWithStatsResult(Model):
 
 
 # Sharing -----------------------------------------------------------------
-class SharingOptions(str, Enum):
-    """Options for sharing resources that may have restricted access to all or part of their contents."""
-
-    make_public = "make_public"
-    make_accessible_to_shared = "make_accessible_to_shared"
-    no_changes = "no_changes"
+# Options for sharing resources that may have restricted access to all or part of their contents.
+SharingOptions = Literal["make_public", "make_accessible_to_shared", "no_changes"]
 
 
 class ShareWithExtra(Model):
@@ -3011,9 +3006,9 @@ class ShareWithPayload(Model):
         description=(
             "User choice for sharing resources which its contents may be restricted:\n"
             " - None: The user did not choose anything yet or no option is needed.\n"
-            f" - {SharingOptions.make_public}: The contents of the resource will be made publicly accessible.\n"
-            f" - {SharingOptions.make_accessible_to_shared}: This will automatically create a new `sharing role` allowing protected contents to be accessed only by the desired users.\n"
-            f" - {SharingOptions.no_changes}: This won't change the current permissions for the contents. The user which this resource will be shared may not be able to access all its contents.\n"
+            " - make_public: The contents of the resource will be made publicly accessible.\n"
+            " - make_accessible_to_shared: This will automatically create a new `sharing role` allowing protected contents to be accessed only by the desired users.\n"
+            " - no_changes: This won't change the current permissions for the contents. The user which this resource will be shared may not be able to access all its contents.\n"
         ),
     )

@davelopez
Copy link
Contributor Author

Multi-literals were not supported in pydantic when generating the OpenAPI schema when we started using FastAPI. Now they are.
Should we convert every enum to multi-literal in this PR?

@mvdbeek
Copy link
Member

mvdbeek commented Mar 8, 2023

I would not do that without a strong reason. I would also be curious to know why Literals are preferred here @nsoranzo

@nsoranzo
Copy link
Member

nsoranzo commented Mar 8, 2023

(str, Enum) is redundant for type annotations, since Literal was introduced for that use case. It also requires much more code and is less readable, both when defining the class and when using the values (SharingOptions.make_public.value vs "value"), with no practical benefit, since a mistyping would be caught by mypy.
It also introduces tricky bugs like the one fixed here or the one fixed in a7ddf89 .

Probably not enough to ask to replace them all, but here yes :)

@mvdbeek
Copy link
Member

mvdbeek commented Mar 8, 2023

I don't think they are exactly equivalent though, they do seem to have slightly different uses in pydantic (though the docs don't go into details, https://docs.pydantic.dev/usage/types/#enums-and-choices and https://docs.pydantic.dev/usage/types/#literal-type)

@davelopez
Copy link
Contributor Author

I guess this is a nice topic for the next WG meeting :)
Sadly I won't be able to attend the meeting in the next couple of weeks 😞

@davelopez
Copy link
Contributor Author

@nsoranzo, are you fine merging this as a fix for building the client API schema in python 3.11?
I can open a new issue for discussion in the backend meeting for replacing all enums with multi-literals if you like. However, I don't think we should hold off merging this fix while waiting for that decision.

@nsoranzo
Copy link
Member

nsoranzo commented Mar 9, 2023

I don't think they are exactly equivalent though, they do seem to have slightly different uses in pydantic (though the docs don't go into details, https://docs.pydantic.dev/usage/types/#enums-and-choices and https://docs.pydantic.dev/usage/types/#literal-type)

The first example of each in these docs are equivalent, but Literal has additional use cases documented.

are you fine merging this as a fix for building the client API schema in python 3.11?
I can open a new issue for discussion in the backend meeting for replacing all enums with multi-literals if you like. However, I don't think we should hold off merging this fix while waiting for that decision.

Sure, it isn't a -1.

@davelopez davelopez added the minor label Mar 9, 2023
@martenson martenson merged commit e37fd5f into galaxyproject:dev Mar 9, 2023
@martenson
Copy link
Member

Thanks @davelopez

@davelopez davelopez deleted the fix_enums_in_api_docs branch March 9, 2023 17:01
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