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

fix: properly type ApplicationContext.author #2148

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

Yuki-42
Copy link
Contributor

@Yuki-42 Yuki-42 commented Jun 25, 2023

Updated author property of ApplicationContext to include typehinting for applicable types.

Summary

This change is intended to assist IDEs like pyCharm in both creating applicable warnings and code completion. This is done to fix The reason that I am trying to add typehinting is without it pycharm kinda just doesn't recognize that the author is an instance of Member or User and throws all kinds of errors when I add proper documentation.

Documentation example:
"""
The trade command for the bot. Allows users to trade with other users.

Args:
    ctx (ApplicationContext): The context of the command
    user (User): The user to trade with
    item (int): The item to trade
    amount (int): The amount of the item to trade
    cost (int): The payment for the trade
"""

Without this change pycharm does not recognize that the author is an instance of Member or User.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

Updated `author` property of `ApplicationContext` to include typehinting for applicable types.

Signed-off-by: Kyle Pannan <[email protected]>
@Yuki-42 Yuki-42 requested a review from a team as a code owner June 25, 2023 09:55
@pullapprove4 pullapprove4 bot requested a review from FrostByte266 June 25, 2023 09:55
@pullapprove4 pullapprove4 bot requested a review from ChickenDevs June 25, 2023 09:55
@plun1331 plun1331 changed the title Update context.py fix: properly type ApplicationContext.author Jun 25, 2023
Copy link
Member

@plun1331 plun1331 left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry

@codecov
Copy link

codecov bot commented Jun 25, 2023

Codecov Report

Merging #2148 (675edc3) into master (297053d) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2148   +/-   ##
=======================================
  Coverage   33.86%   33.86%           
=======================================
  Files         109      109           
  Lines       22367    22367           
=======================================
  Hits         7575     7575           
  Misses      14792    14792           
Flag Coverage Δ
macos-latest-3.10 33.84% <100.00%> (ø)
macos-latest-3.11 33.84% <100.00%> (ø)
macos-latest-3.8 33.85% <100.00%> (ø)
macos-latest-3.9 33.85% <100.00%> (ø)
ubuntu-latest-3.10 33.84% <100.00%> (ø)
ubuntu-latest-3.11 33.84% <100.00%> (ø)
ubuntu-latest-3.8 33.85% <100.00%> (ø)
ubuntu-latest-3.9 33.85% <100.00%> (ø)
windows-latest-3.10 33.84% <100.00%> (ø)
windows-latest-3.11 33.84% <100.00%> (ø)
windows-latest-3.8 33.85% <100.00%> (ø)
windows-latest-3.9 33.85% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
discord/commands/context.py 50.34% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 297053d...675edc3. Read the comment docs.

@Yuki-42
Copy link
Contributor Author

Yuki-42 commented Jun 25, 2023

Please add a changelog entry

Would it go in added, changed, or fixed?

@plun1331
Copy link
Member

Please add a changelog entry

Would it go in added, changed, or fixed?

I would say fixed since it's fixing bad typing, possibly added since there was no typehint to begin with

@JustaSqu1d
Copy link
Member

JustaSqu1d commented Jun 26, 2023

Please add a changelog entry

Would it go in added, changed, or fixed?

I would say fixed since it's fixing bad typing, possibly added since there was no typehint to begin with

In my opinion, it makes the most sense for "fixed," as well.

@Yuki-42
Copy link
Contributor Author

Yuki-42 commented Jun 26, 2023

I'm assuming something along the lines of

  • Fixed type-hinting of author property of ApplicationContext to include typehinting of User or Member.
    (#2148)

would be fine?

@Lulalaby
Copy link
Member

Yes perfectly fine

@Yuki-42
Copy link
Contributor Author

Yuki-42 commented Jun 26, 2023

I believe I have added the changelog entry correctly.

@Lulalaby Lulalaby enabled auto-merge (squash) June 26, 2023 08:35
@Lulalaby Lulalaby requested a review from plun1331 June 26, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working changelog needed status: in progress Work in Progess
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants