Skip to content

Add typings #1497

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

Closed
wants to merge 193 commits into from
Closed

Add typings #1497

wants to merge 193 commits into from

Conversation

bryanforbes
Copy link
Contributor

@bryanforbes bryanforbes commented Aug 10, 2018

I've added pyi files as siblings to all of the library files:

  • I've tried to only type the public API. I may have missed something or provided too many methods. Feel free to indicate what should be added or removed.
  • Any class that is typed as generic (iterators, Command, Group, etc.) has to inherit from typing.Generic in order to be subscriptable and not throw errors when using generic subscripts in python code.
  • In the course of doing this work, I converted the entire core codebase over to typed Python and found a few bugs. I've included those bug fixes (calls.py, message.py, state.py, and user.py).
  • While Python 3.5.x is still the oldest version the library uses, the typings require Python 3.6 for PEP-526.
  • I've added an optional feature that will pull in mypy_extensions and typing_extensions for TypedDict and Protocol respectively. These will not be pulled in unless specified by the consumer.
  • I have also tested these typings in a library I wrote that uses discord.py as well as a bot I wrote.

@Rapptz
Copy link
Owner

Rapptz commented Aug 23, 2018

I understand the usage and reasons for .pyi files existing.

However, I do not think they apply to this right? Shouldn't the goal be to apply these typings to the main functions inside the library instead of using stubs?

@bryanforbes
Copy link
Contributor Author

There are a few advantages to using .pyi files alongside modules:

  • Keeping typings out of the main library gives a small startup performance gain.
  • Typing the function signatures in the library will cause mypy to type check the function body when it is run on the library code base.
  • Only exposing the public API of the library.

As I said, I initially started out typing the entire library (including the function bodies), but after discussing with you in PM on Discord you mentioned that you didn't feel strongly about having the library code typed. I thought this was a good compromise.

@Hornwitser
Copy link
Contributor

The bugfixes should probably be split up into their own commit (or maybe pull request). They don't really have anything to do with adding typings to the lib.

@bryanforbes
Copy link
Contributor Author

The bugfixes should probably be split up into their own commit (or maybe pull request).

Good call. I'll separate them out.

@bryanforbes
Copy link
Contributor Author

bryanforbes commented Aug 25, 2018

I've separated the fixes out into their own PRs and removed them from this one as well as rebasing on rewrite

Copy link

@Werseter Werseter left a comment

Choose a reason for hiding this comment

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

BotBase type hints could use the Dict[str, Any] for cogs, extra_events and extensions.

Also, any call to discord.ext.commands going through such syntax complains about ext not being in the __init__.pyi.

@bryanforbes
Copy link
Contributor Author

BotBase type hints could use the Dict[str, Any] for cogs, extra_events and extensions.

Added

Also, any call to discord.ext.commands going through such syntax complains about ext not being in the __init__.pyi.

As long as you import discord.ext.commands, you can use discord.ext.commands.Bot(). Even without typings, this is the case. You cannot access discord.ext.commands by simply using import discord.

@Werseter
Copy link

Werseter commented Sep 7, 2018

If you do from discord.ext import commands and call discord.ext.commands for some reason instead of just commands it will prompt up as not setup up for typing (also implicitly assuming discord has been imported as well)

I never implied it is a proper way to use it, but it exists, and typings should cover for it too, instead of pulling up a warn.

@bryanforbes
Copy link
Contributor Author

I just tried the following:

import discord
from discord.ext import commands

Bot = discord.ext.commands.Bot
reveal_type(Bot)

I don't get a warning from mypy and it correctly reveals that it's a function that will return discord.ext.commands.bot.Bot[CT]. This seems to be the scenario you're describing and it seems to work.

@Werseter
Copy link

Werseter commented Sep 8, 2018

Could it be it is the PyCharm's linter issue?
image

That would be silly tho, but if it's fine with mypy, then I guess it's fine, it's a niche case anyway.

@Werseter
Copy link

It's been a while, as I still am in middle of transitioning my project to Type Hints, but I caught that abc.GuildChannel, abc.Messageable, abc.PrivateChannel and abc.User, could use Snowflake hints, as per "This ABC must also implement :class:abc.Snowflake."

@bryanforbes
Copy link
Contributor Author

Since Snowflake is a Protocol, any object that has the same properties as it has will match it, which is what the actual Snowflake class does. The classes which subclass the ABCs you listed all implement the Snowflake protocol. The documentation string should probably be updated to say “Subclasses of this ABC must also implement the Snowflake protocol”.

@Werseter
Copy link

Well, given that any object typed as GuildChannel - i.e list returned by ctx.guild.channels, will be said to not have an id property, as it must be implemented by a subclass, but not by the abstract itself. Since the assumption of this implementation is declared as imperative, shouldn't the typings have those two properties included?

@Werseter
Copy link

Werseter commented Sep 19, 2018

Or, perhaps the other way around, possibly the channels field of Guild could be typed as List[Union[TextChannel, VoiceChannel, CategoryChannel]] instead of List[GuildChannel].

This would merely be escaping from the single-case issue here. It raises a question of whether anything should be typed with an abstract, and not with the actual functional classes.

@Werseter
Copy link

Werseter commented Sep 25, 2018

Just a note about the fact recent library update might require this getting a lil update.

@nihaals
Copy link
Contributor

nihaals commented Nov 6, 2018

Is this going to be merged or should someone change it to be in code (like using Sphinx doc strings allowing it to be explicit on the docs too)? I've wanted typing for the last year so would love a solution

@Werseter
Copy link

Werseter commented Nov 6, 2018 via email

@nihaals
Copy link
Contributor

nihaals commented Nov 6, 2018

I think most of the attributes missing types have a doc string anyway, so although it seems a bit long, the be might be using Returns as if it was a function, and maybe in the future convert some of it to another solution.

@ShineyDev ShineyDev mentioned this pull request Jul 17, 2020
@ghost
Copy link

ghost commented Sep 1, 2020

Any news on when this will be added to discord.py? Typing is very useful.

@Jackenmen
Copy link
Contributor

I don't have a news on that, but now there's a discord.py-stubs package from the author of this PR, which you could use if you need d.py to be typed.

@bryanforbes
Copy link
Contributor Author

After a few months of inactivity here, I've decided that it will be easier for me to maintain the stubs in their own package on pypi named discord.py-stubs. This does add some complexity in using discord.py with mypy, namely using generics from the stubs when they aren't generic at runtime. For instance, to create a Cog that could be type checked you would have to do the following:

from typing import TYPE_CHECKING
from discord.ext import commands

if TYPE_CHECKING:
    Cog = commands.Cog[commands.Context]
else:
    Cog = commands.Cog

class MyCog(Cog):
    ...

To alleviate this issue, I have also created discord-ext-typed-commands which mirrors the commands module, but adds the ability to subscript for generics:

from discord.ext import typed_commands

class MyCog(typed_commands.Cog[commands.Context]):
    ...

Thanks to @Rapptz for the help here as well as everyone who reviewed this PR (especially @NCPlayz and @Werseter )!

@Rapptz Rapptz removed the needs review This PR will be reviewed in the future label Jan 16, 2025
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.