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

refactor: use dict {} syntax for typing #541

Merged
merged 1 commit into from
Feb 28, 2025
Merged

Conversation

joseph-sentry
Copy link
Contributor

getting a type error with the TypedDict when using the dict syntax to create the ProviderPull in github.py and using the {} syntax fixes this

getting a type error with the TypedDict when using the dict syntax
to create the ProviderPull in github.py and using the {} syntax fixes
this
Copy link

codspeed-hq bot commented Feb 26, 2025

CodSpeed Performance Report

Merging #541 will create unknown performance changes

Comparing joseph/quick-typing-fix (8986268) with main (4de8f9e)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.\

@joseph-sentry joseph-sentry requested a review from a team February 27, 2025 18:51
Copy link
Contributor

@adrian-codecov adrian-codecov left a comment

Choose a reason for hiding this comment

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

Wait really? What makes the non-dict approach work w/ typedict? If the dict syntax is failing, isn't this indicative that we're miss-capturing a type or something?

Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

I think the dict fn obscures the resulting type, whereas the literal makes it very obvious what it is.

@joseph-sentry joseph-sentry added this pull request to the merge queue Feb 28, 2025
Merged via the queue into main with commit bc3dd8d Feb 28, 2025
8 checks passed
@joseph-sentry joseph-sentry deleted the joseph/quick-typing-fix branch February 28, 2025 14:31
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