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

[WIP,RFC] Convert Enums to Literals in schema #15774

Closed
wants to merge 5 commits into from

Conversation

nsoranzo
Copy link
Member

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

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

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

@nsoranzo nsoranzo added the kind/refactoring cleanup or refactoring of existing code, no functional changes label Mar 12, 2023
@jdavcs jdavcs changed the title [WIP] Conver Enums to Literals in schema [WIP] Convert Enums to Literals in schema Mar 13, 2023
@nsoranzo nsoranzo added this to the 23.1 milestone Mar 14, 2023
@jmchilton
Copy link
Member

I am fine with the change - because as a purely practical matter pydantic is clunky with enums - but I think a lot of the code you have in there was more readable before and the mypy does a better job with enums in some important ways so I wanted to push back on "with no practical benefit".

Take for instance:

Before:

elif job_source_type == JobSourceType.ImplicitCollectionJobs:

After:

elif job_source_type == "ImplicitCollectionJobs":

To me - the intent reads a little more clear with the first line. I think the mypy side of things is a bit more objective though. A lot of these lines in the diff are just comparing a variable (typed or not) against some constant string. static checking would catch if the import of JobSourceType is off or if ImplicitCollectionJobs was not a member of that type. However, since the type of == is effectively (any, any) -> bool - mypy and IDEs are totally fine with typos in the literal comparisons. For instance, mypy doesn't care about a classically @jmchilton typo such as:

elif job_source_type == "ImplicitCollecitonJobs":

but would both catch the comparable:

elif job_source_type == JobSourceType.ImplicitCollecitonJobs:

(In other news, I finally installed a spell checker in my IDE over last winter break so the damage @jmchilton typos will do should be on a downward trend regardless of mypy.)

Showing my work:

% cat test_literal.py 
from typing import Literal

seven_eight_nine = Literal["7", "8", "9"]
seven: seven_eight_nine = "7"

assert seven == "6" is True
% cat test_enum.py 
from enum import Enum

class SevenEightNine(str, Enum):
    seven = "7"
    eight = "8"
    nine = "9"

seven: SevenEightNine = SevenEightNine("7")

assert seven == SevenEightNine.six is True
% mypy test_literal.py 
Success: no issues found in 1 source file
% mypy test_enum.py 
test_enum.py:10: error: "Type[SevenEightNine]" has no attribute "six" 
[attr-defined]
    assert seven == SevenEightNine.six is True
                    ^~~~~~~~~~~~~~~~~~
Found 1 error in 1 file (checked 1 source file)

@nsoranzo nsoranzo changed the title [WIP] Convert Enums to Literals in schema [WIP,RFC] Convert Enums to Literals in schema Mar 15, 2023
@nsoranzo
Copy link
Member Author

nsoranzo commented Mar 15, 2023

Thanks a lot @jmchilton for having a look and for the very insightful comment!
I agree that for typos in comparisons this could be an issue, BUT I've found (on https://adamj.eu/tech/2021/07/09/python-type-hints-how-to-use-typing-literal/#comparisons ) that there's a --strict-equality option of mypy that flags the error:

$ mypy --strict-equality test_literal.py 
test_literal.py:6: error: Non-overlapping equality check (left operand type: "Literal['7', '8', '9']", right operand type: "Literal['6']")  [comparison-overlap]
Found 1 error in 1 file (checked 1 source file)

The --strict-equality should be easy to add, it founds only 2 issues in the galaxy codebase, see #15808 .

*/
job_source_type?: components["schemas"]["JobSourceType"];
job_source_type?: "Job" | "ImplicitCollectionJobs" | "WorkflowInvocation";
Copy link
Member

Choose a reason for hiding this comment

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

That is a big downside IMO, making reuse harder.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 15, 2023

I think I'm still pretty strongly against this for pydantic model fields, which are not serialized in the same way.

@nsoranzo nsoranzo closed this Mar 15, 2023
martenson added a commit to martenson/galaxy that referenced this pull request Jun 7, 2023
martenson added a commit to martenson/galaxy that referenced this pull request Jun 8, 2023
see galaxyproject#15774 for context

update page =pdf export selenium test

update return type to reflect matches being returned

cleanup the test file

run make format
martenson added a commit to martenson/galaxy that referenced this pull request Jun 14, 2023
see galaxyproject#15774 for context

update page =pdf export selenium test

update return type to reflect matches being returned

cleanup the test file

run make format
@nsoranzo nsoranzo deleted the Enum_to_Literal branch July 21, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API area/UI-UX kind/refactoring cleanup or refactoring of existing code, no functional changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants