Skip to content

Conversation

@EepyElvyra
Copy link
Contributor

@EepyElvyra EepyElvyra commented May 24, 2022

About

This is a new approach for the get method. It is now located in a separate file, so the @overloads don't mess another file up. Having an overload decorator for every object allows to precisely show the user what arguments he has to put in, and for us to do a simple **kwagrs into the function that actually gets the object

Checklist

  • I've ran pre-commit to format and lint the change(s) made.
  • I've checked to make sure the change(s) work on 3.8.6 and higher.
  • This fixes/solves an Issue.
    • (If existent):
  • I've made this pull request for/as: (check all that apply)
    • Documentation
    • Breaking change
    • New feature/enhancement
    • Bugfix

@EepyElvyra
Copy link
Contributor Author

EepyElvyra commented May 24, 2022

pre commit check will (hopefully) fail until webhook pr is merged

@EepyElvyra EepyElvyra requested a review from Toricane May 25, 2022 11:40
Copy link
Contributor

@Toricane Toricane left a comment

Choose a reason for hiding this comment

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

I didn't mean that...

Copy link
Contributor

@Toricane Toricane left a comment

Choose a reason for hiding this comment

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

Looks good, just missing cache implementation.

Also, should getting a list of an object be implemented, or is that out of the scope of this?
For example, List[User], with a list of user IDs inputted, which returns a list of User objects.

@EepyElvyra EepyElvyra closed this Jul 4, 2022
@EepyElvyra
Copy link
Contributor Author

I hate everything xD

@EepyElvyra EepyElvyra reopened this Jul 4, 2022
@EepyElvyra EepyElvyra requested a review from AstreaTSS July 4, 2022 22:24
Copy link
Member

@AstreaTSS AstreaTSS left a comment

Choose a reason for hiding this comment

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

I find the number of double underscored variables to be excessive and unnecessary in this case, but that's a me thing.

Otherwise, the code looks good! I haven't tested it though - busy with some other things. I also likely have missed a couple of things, which is why I'm not giving it explicit approval.

@EepyElvyra
Copy link
Contributor Author

I find the number of double underscored variables to be excessive and unnecessary in this case, but that's a me thing.

Otherwise, the code looks good! I haven't tested it though - busy with some other things. I also likely have missed a couple of things, which is why I'm not giving it explicit approval.

I only made double underscores when single underscore was used already, no? (except the two functions)

@EepyElvyra EepyElvyra enabled auto-merge (squash) July 11, 2022 16:40
@EepyElvyra EepyElvyra requested review from Catalyst4222 and i0bs July 11, 2022 19:25
@i0bs
Copy link
Contributor

i0bs commented Jul 12, 2022

Before I make a final review, I'd really like to discuss my disdain for @overload.

This is going to obviously lead to an extremely messy and obfuscated pattern in the type hinting for numerous objects. Is there not any other way to assure to end-users that objects passed by the Discord API/Gateway are usable in this context?

@EepyElvyra
Copy link
Contributor Author

Before I make a final review, I'd really like to discuss my disdain for @overload.

This is going to obviously lead to an extremely messy and obfuscated pattern in the type hinting for numerous objects. Is there not any other way to assure to end-users that objects passed by the Discord API/Gateway are usable in this context?

There are 9 overloads (what already is a massive reduction from the 36 we had before) and the typehints can be controlled via the _SA and _MA TypeVars, i don't think that it is too messy

Copy link
Contributor

@i0bs i0bs left a comment

Choose a reason for hiding this comment

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

Long awaited, and after nearly a year of debating how to develop this, I'm giving it my seal of approval.

@AstreaTSS AstreaTSS dismissed their stale review July 13, 2022 14:12

Outdated

Copy link
Contributor

@Toricane Toricane left a comment

Choose a reason for hiding this comment

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

LGTM!

@EepyElvyra EepyElvyra merged commit 61a3017 into interactions-py:unstable Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants