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

Automatically add type hints for API Object attributes #141

Merged
merged 2 commits into from
Jun 12, 2024
Merged

Conversation

shrik450
Copy link
Collaborator

@shrik450 shrik450 commented Jun 11, 2024

Adds a script, scripts/generate_attribute_types.py, which automatically generates type hints for attributes in API objects. Also adds the generated type hints, and a test workflow step to check that types are up to date.

There are two main motivations for this:

  1. Users of py-allspice don't know which fields are in the classes. This means they have to interactively experiment with py-allspice to figure out what they can use. This fixes that by providing type hints for the attributes in the class, which means they can also type check their code!
  2. We need to type check py-allspice to catch bugs before running tests. See Should pass mypy type-checking #36. This PR doesn't enable type-checking, as there are likely to be many failures right now, and fixing them is out of scope for now.

The type generation scripts runs the test_api test suite, profiles it to see the actual types of the attributes at runtime, and uses that to patch the module file with those actual types. This is inspired by (and uses the internals of) MonkeyType. MonkeyType does not do attribute types, which is why we had to hand-roll this implementation. The module patching functionality uses LibCST, and also patches missing functionality in LibCST, namely Instagram/LibCST#323 and Instagram/LibCST#1159.

The reason this is so complicated is that all of the classes in apiobject.py have attributes dynamically injected at runtime from the JSON response returned by AllSpice Hub. These can change with every version of AllSpice Hub. So, any system to generate types must be able to detect when types are changed upstream and update them. This script fulfils that, and in fact this PR adds a CI check to ensure that the types that are checked in match what ASH is sending.

There are still a few rough edges in this system:

  1. Our test suite often leaves some attributes unset because we aren't testing comprehensively enough. This means there's a lot of Anys.
  2. I set a hard stop for this task, which led to a couple of hacky fixes in the code, such as the manual rewriting of the datetime type.
  3. Some types are very complex, and we need to add parsers to make those types simpler for the user to handle.
  4. We need to format and lint the changed code, since the script sometimes writes incorrectly formatted code.

But all of that can be fixed with more work in the future, and the script is plenty useful as is.

@shrik450 shrik450 requested review from jtran and kdumontnu June 11, 2024 16:13
@shrik450 shrik450 requested a review from a team as a code owner June 11, 2024 16:13
@shrik450 shrik450 self-assigned this Jun 11, 2024
Required to run the pytest test suite within the script
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jtran jtran left a comment

Choose a reason for hiding this comment

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

💯

@jtran
Copy link
Contributor

jtran commented Jun 12, 2024

I noticed that CI generates the types and checks to see that they haven't changed. Do you have a plan for how we'll improve types, e.g. replacing Any with the correct type? Is the only way to do that by creating a test that uses that field with a real value?

@shrik450
Copy link
Collaborator Author

I noticed that CI generates the types and checks to see that they haven't changed. Do you have a plan for how we'll improve types, e.g. replacing Any with the correct type? Is the only way to do that by creating a test that uses that field with a real value?

Yes, that's the plan. It's a little annoying, but it's good for two reasons:

  1. We have to write tests that actually bring that information in, or create a parser to make a complex type simple
  2. It's simpler to automate the check

For a while I considered checking if the annotated type was a subtype of the generated type when checking, but this was a little difficult to do as a whole and simple text diffing gets us most of the way. I can still pick that up in the future.

@shrik450 shrik450 merged commit c42e28a into main Jun 12, 2024
4 checks passed
@shrik450 shrik450 deleted the su/types branch June 12, 2024 17:47
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.

3 participants