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

Simplify PEP cog to use PEP API #2166

Closed
wants to merge 3 commits into from
Closed

Simplify PEP cog to use PEP API #2166

wants to merge 3 commits into from

Conversation

wookie184
Copy link
Contributor

@wookie184 wookie184 commented May 2, 2022

There is a new PEP API (https://peps.python.org/api/peps.json) that saves us having to manually scrape files from github, largely simplifying the implementation.

As far as I can tell this was also the only usage of https://github.com/python-discord/bot/blob/5fe110b9a49144480ebca34dec65de91753994ec/bot/utils/caching.py, do we want to remove that given it's no longer used?

@wookie184 wookie184 requested review from Den4200 and jb3 as code owners May 2, 2022 15:34
@ChrisLovering
Copy link
Member

As far as I can tell this was also the only usage of https://github.com/python-discord/bot/blob/5fe110b9a49144480ebca34dec65de91753994ec/bot/utils/caching.py, do we want to remove that given it's no longer used?

I'd vote for removing it yea. We could add it to bot-core for future use if needed too.

@wookie184 wookie184 added a: information Related to information commands: (doc, help, information, reddit, site, tags) t: enhancement Changes or improvements to existing features s: needs review Author is waiting for someone to review and approve p: 3 - low Low Priority labels May 2, 2022
@vivekashok1221 vivekashok1221 self-requested a review May 2, 2022 18:00
Copy link
Member

@vivekashok1221 vivekashok1221 left a comment

Choose a reason for hiding this comment

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

Wonderful work, wookie. I've reviewed and tested, I just have one small question.

bot/exts/info/pep.py Show resolved Hide resolved
Copy link
Member

@vivekashok1221 vivekashok1221 left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

@wookie184 wookie184 added the review: do not merge The PR can be reviewed but cannot be merged now label May 7, 2022
@wookie184
Copy link
Contributor Author

There's no rush to get this merged so we should wait for python/peps#2584 to be resolved.

@CAM-Gerlach
Copy link

Yeah, I'd suggest waiting if you can, since until #2584 is resolved (hopefully soon...™) the "API" is experimental and undocumented, and the path/endpoint, structure, etc. may change.

@wookie184 wookie184 added s: stalled Something is blocking further progress and removed s: needs review Author is waiting for someone to review and approve review: do not merge The PR can be reviewed but cannot be merged now labels May 28, 2022
@wookie184 wookie184 marked this pull request as draft May 28, 2022 16:27
Copy link
Member

@shtlrs shtlrs left a comment

Choose a reason for hiding this comment

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

Looks great, thanks wookie :D
One tiny comment for you :p

self.peps: Dict[int, str] = {}
# To avoid situations where we don't have last datetime, set this to now.
self.last_refreshed_peps: datetime = datetime.now()
self.peps: dict[int, dict[str, Optional[str]]] = {}
Copy link
Member

Choose a reason for hiding this comment

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

This would be a good place to start using PEP 604 :D

@wookie184
Copy link
Contributor Author

I'm going to close this PR for now as it's been stalled for a while, hopefully it can be reused at some point in the future.

@wookie184 wookie184 closed this Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: information Related to information commands: (doc, help, information, reddit, site, tags) p: 3 - low Low Priority s: stalled Something is blocking further progress t: enhancement Changes or improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants