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

[MAINTENANCE] Type hint cleanup in usage statistics #8105

Merged
merged 5 commits into from Jun 13, 2023

Conversation

cdkini
Copy link
Member

@cdkini cdkini commented Jun 13, 2023

  • Description of PR changes above includes a link to an existing GitHub issue
  • PR title is prefixed with one of: [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], [CONTRIB]
  • Code is linted - run invoke lint (uses black + ruff)
  • Appropriate tests and docs have been updated

For more details, see our Contribution Checklist, Coding style guide, and Documentation style guide.

After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!

@netlify
Copy link

netlify bot commented Jun 13, 2023

Deploy Preview for niobium-lead-7998 canceled.

Name Link
🔨 Latest commit e5059fb
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/64886e3bd67f7900086dddd2

@github-actions github-actions bot added the core label Jun 13, 2023
@ghost
Copy link

ghost commented Jun 13, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@cdkini cdkini self-assigned this Jun 13, 2023
@cdkini cdkini requested a review from Kilo59 June 13, 2023 13:15
Comment on lines +230 to +233
func: Callable | None = None,
event_name: UsageStatsEvents | None = None,
args_payload_fn: Callable | None = None,
result_payload_fn: Callable | None = None,
Copy link
Member

Choose a reason for hiding this comment

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

Is there anything else about the callable that we can declare here?
Like what they are expected to return or the params they accept?

Copy link
Member Author

Choose a reason for hiding this comment

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

unfortunately its a super variable input list (the fn in question here is any method we decorate)

Copy link
Member Author

Choose a reason for hiding this comment

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

if you look at the context, we have a number of varied usage stats decorated methods

Copy link
Member

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

❤️

@cdkini cdkini enabled auto-merge (squash) June 13, 2023 13:20
@cdkini cdkini merged commit a2a4d22 into develop Jun 13, 2023
48 checks passed
@cdkini cdkini deleted the m/lakitu-211/type_hint_cleanup branch June 13, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants