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

Use string enum EntityType #65

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

disinvite
Copy link
Collaborator

@disinvite disinvite commented Jan 7, 2025

Note: StrEnum requires python 3.11 or later, so this effectively becomes our minimum version if this is merged. CI actions here and in isledecomp/isle use 3.12.


Resolves #51.

As of #45, items in the database (i.e. the components of the binaries) are "entities". This renames SymbolType to EntityType to match. It is a StrEnum to support setting the value via a string (see #32). We use enum.auto() to establish the values. The expected behavior is that the raw value is a lower case string version of the enum constant.

There are three other changes. The main one is the entity_type property method of ReccmpEntity (formerly compare_type). It now always returns an EntityType. If none is set, return EntityType.UNKNOWN. If the raw value in the database is not part of the enum, return EntityType.ERRTYPE. This seems appropriate because this method is a helper anyway, and our current code assumes that an enum is returned. You can get the raw value with the .get() method.

match_name() from ReccmpEntity now takes advantage of the fact that compare_type is always an enum and so we use its string value directly.

Similarly, match_type_abbreviation() from roadmap uses the first three characters of the enum value.

@disinvite disinvite marked this pull request as ready for review January 7, 2025 20:30
@jonschz
Copy link
Collaborator

jonschz commented Jan 7, 2025

Disregarding the CI, doesn't this also break Python < 3.11 support for running locally? I don't have any personal stakes in supporting older, but still maintained Python versions, but I'm not sure we have a team agreement on which version are supposed to be supported.

Edit: My bad, should have checked the chat before commenting. Leaving it here for documentation.

@disinvite
Copy link
Collaborator Author

Maybe the python version question should be its own PR rather than jammed into this one. If we're saying 3.11 is the minimum then we should probably test on that version (along with newer ones).

@disinvite disinvite marked this pull request as draft January 8, 2025 04:04
@disinvite
Copy link
Collaborator Author

disinvite commented Jan 8, 2025

I might break this up into a few mutually exclusive PRs for simplicity. We don't absolutely need to use strings to allow loading metadata from a file. We can just read the string "vtable" and output EntityType.VTABLE even if the enum is still an int behind the scenes.

The items in flight are:

  • Rename SymbolType to EntityType and compare_type property to entity_type. May as well rename the SQL table to "entities" while we're at it.
  • entity_type property always returns enum instance.
  • Run CI tests against multiple python versions.
  • Establish minimum Python version. If we will use StrEnum, that's 3.11. If not, it's 3.10.
  • "Format" task should use whatever minimum python version. The other tasks already use 3.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconsider SymbolType enum
2 participants