Skip to content

Fix type annotations#802

Merged
Xithrius merged 11 commits into
mainfrom
decorator-factory/typehints-fix
Sep 2, 2021
Merged

Fix type annotations#802
Xithrius merged 11 commits into
mainfrom
decorator-factory/typehints-fix

Conversation

@decorator-factory
Copy link
Copy Markdown
Contributor

@decorator-factory decorator-factory commented Aug 7, 2021

This PR aims to improve type annotations (type hints) in the project:

  • change old-style annotations (like typing.List[int]) to new-style annotations (like list[int])
  • fix annotations that are just wrong

Copy link
Copy Markdown
Contributor

@kwzrd kwzrd 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, I think I only caught 1 or 2 things that could mistakes, but you be the judge of that.

I approach this as a PR that intends to make better use of PEP 585. Therefore I was mostly looking for regression; I haven't verified that all existing hints are correct.

Comment thread bot/exts/evergreen/snakes/_utils.py
Comment thread bot/exts/evergreen/snakes/_utils.py Outdated
Comment thread bot/exts/evergreen/snakes/_utils.py
Comment thread bot/exts/evergreen/movie.py Outdated
Comment thread bot/exts/evergreen/wolfram.py
Copy link
Copy Markdown
Contributor

@kwzrd kwzrd left a comment

Choose a reason for hiding this comment

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

Looks LGTM to me, good changes overall.

The notion of imposing better validation on data coming from external sources is attractive, though on the other hand Lancebot is primarily aimed at beginners. I don't know whether it'd make sense to put in the effort of making existing features more resilient if the same level of resilience isn't required from new contributions.

@decorator-factory decorator-factory marked this pull request as ready for review August 8, 2021 15:32
@decorator-factory
Copy link
Copy Markdown
Contributor Author

decorator-factory commented Aug 8, 2021

I think it's a good idea to teach beginners about validation, security and all that stuff. Lancebot being a learning project, we want to teach people good practices. Besides, the bot is running on our hardware and has access to some of our secrets, so we wouldn't want to introduce vulnerabilities (#801)

Comment thread bot/exts/evergreen/snakes/_converter.py Outdated
@Xithrius Xithrius added area: backend Related to internal functionality and utilities status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features labels Aug 16, 2021
@Xithrius
Copy link
Copy Markdown
Contributor

I will be continuing this PR, as @decorator-factory authorized.

@Xithrius Xithrius self-assigned this Aug 22, 2021
@Xithrius Xithrius added status: WIP Work In Progress and removed status: needs review Author is waiting for someone to review and approve labels Aug 22, 2021
Copy link
Copy Markdown

@ventaquil ventaquil left a comment

Choose a reason for hiding this comment

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

Can some explain why sometimes it's import typing, sometimes it's from typing import ... and even sometimes it's import typing as t? 🤔

Comment thread bot/exts/easter/bunny_name_generator.py Outdated
Comment thread bot/exts/christmas/advent_of_code/_helpers.py Outdated
Comment thread bot/exts/evergreen/fun.py Outdated
Copy link
Copy Markdown
Contributor

@brad90four brad90four left a comment

Choose a reason for hiding this comment

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

LGTM. That was a lot of different files to go through!

@Xithrius
Copy link
Copy Markdown
Contributor

I'll be getting this done soon, currently having IRL stuff.

Comment thread bot/exts/utils/extensions.py Outdated
Comment thread bot/exts/internal_eval/_helpers.py Outdated
Comment thread bot/exts/internal_eval/_helpers.py Outdated
Comment thread bot/exts/internal_eval/_helpers.py Outdated
Comment thread bot/exts/internal_eval/_helpers.py Outdated
Comment thread bot/exts/evergreen/connect_four.py Outdated
Comment thread bot/exts/evergreen/connect_four.py Outdated
Comment thread bot/exts/evergreen/cheatsheet.py Outdated
Comment thread bot/exts/christmas/advent_of_code/_helpers.py Outdated
Comment thread bot/exts/christmas/advent_of_code/_helpers.py Outdated
@Xithrius
Copy link
Copy Markdown
Contributor

I intend to start working on this within the next 24 hours.

@Xithrius Xithrius removed the status: WIP Work In Progress label Aug 30, 2021
@Xithrius
Copy link
Copy Markdown
Contributor

Xithrius commented Aug 31, 2021

I will be squashing this PR when it's done.

@Xithrius Xithrius requested review from MarkKoz, jb3 and ventaquil August 31, 2021 00:55
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

  • bot/exts/__init__.py should import Iterator from collections.abc instead.
  • bot/utils/extensions.py should import Iterator from collections.abc instead.

Comment thread bot/utils/checks.py Outdated
Comment thread bot/exts/utils/extensions.py Outdated
Comment thread bot/utils/decorators.py Outdated
Comment thread bot/utils/randomization.py Outdated
Comment thread bot/exts/evergreen/wolfram.py Outdated
@Xithrius Xithrius force-pushed the decorator-factory/typehints-fix branch from 55ecb2a to 745cd1d Compare August 31, 2021 20:09
Comment thread bot/utils/decorators.py Outdated
Comment thread bot/utils/decorators.py Outdated
Comment thread bot/utils/decorators.py Outdated
Comment thread bot/utils/randomization.py Outdated
Comment thread bot/exts/internal_eval/_helpers.py
Copy link
Copy Markdown
Member

@Den4200 Den4200 left a comment

Choose a reason for hiding this comment

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

Just a few minor things.

Comment thread bot/exts/evergreen/coinflip.py Outdated
Comment thread bot/exts/evergreen/wikipedia.py Outdated
Comment thread bot/exts/easter/bunny_name_generator.py
Comment thread bot/exts/christmas/advent_of_code/_cog.py
Comment thread bot/exts/evergreen/battleship.py Outdated
Comment thread bot/exts/evergreen/snakes/_utils.py
Comment thread bot/exts/evergreen/wolfram.py Outdated
Comment thread bot/utils/randomization.py
Comment thread bot/exts/evergreen/connect_four.py Outdated
Copy link
Copy Markdown
Member

@MrHemlock MrHemlock left a comment

Choose a reason for hiding this comment

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

Everything looks solid. I didn't see anything missed.

@Xithrius Xithrius requested a review from fisher60 September 2, 2021 18:55
Comment thread bot/exts/christmas/advent_of_code/_helpers.py
Copy link
Copy Markdown
Contributor

@jonathan-d-zhang jonathan-d-zhang left a comment

Choose a reason for hiding this comment

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

lgtm, but why

Copy link
Copy Markdown
Contributor

@fisher60 fisher60 left a comment

Choose a reason for hiding this comment

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

looks pretty good to me I think

@Xithrius Xithrius merged commit 8813045 into main Sep 2, 2021
@Xithrius Xithrius deleted the decorator-factory/typehints-fix branch September 2, 2021 19:42
@Xithrius
Copy link
Copy Markdown
Contributor

Xithrius commented Sep 2, 2021

I didn't squash, sorry about that.

@Bluenix2 Bluenix2 mentioned this pull request Sep 8, 2021
4 tasks
@TizzySaurus TizzySaurus mentioned this pull request Dec 23, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: backend Related to internal functionality and utilities type: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.