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

Add Hacktoberfest user stats cog #7

Merged
merged 7 commits into from
Oct 7, 2018
Merged

Add Hacktoberfest user stats cog #7

merged 7 commits into from
Oct 7, 2018

Conversation

sco1
Copy link
Contributor

@sco1 sco1 commented Oct 4, 2018

Initial functionality for #6 for review.

There are a few typos to fix but they'll be squashed in with changes from review.

image

bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved

Otherwise, return None
"""
queryURL = f"https://api.github.com/search/issues?q=-label:invalid+type:pr+is:public+author:{username}+created:2018-10-01..2018-10-31&per_page=300"
Copy link
Member

Choose a reason for hiding this comment

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

Can this be split into multiple lines? maybe one new line for each new filter on the search.

Copy link
Contributor Author

@sco1 sco1 Oct 5, 2018

Choose a reason for hiding this comment

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

Something like:

base_url = "https://api.github.com/search/issues?q="
not_label = "invalid"
action_type = "pr"
is_query = f"public+author:{username}"
date_range = "2018-10-01..2018-10-31"
per_page = "300"
query_url = f"{base_url}-label:{not_label}+type:{action_type}+is:{is_query}+created:{date_range}&per_page={per_page}"

Or should I go with a concatenated multi-line string?

bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
Copy link
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

Great PR! Just needs some cleaning up for readability, particularly lowercase variable, argument and method names.

@jb3
Copy link
Member

jb3 commented Oct 5, 2018

Might want to run this through flake8 to clear up some of the syntactical errors I missed in the reviews.

Adjust variable & function names to be more in line with PEP8's
recommendations.

Break API query URL into multiple parts to aid readability and
customization

Adjust command input to explicitly accept a username input rather than
star packing & unpacking. d.py guards against too many inputs for us

Change _getURL to _get_shortname and adjust documentation because we're
not even doing what the function name and documentation said we were
Copy link
Member

@jb3 jb3 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!

bot/cogs/hacktoberstats.py Outdated Show resolved Hide resolved
Copy link
Member

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

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

well that's just nice isn't it.

@jb3 jb3 merged commit bee600f into python-discord:master Oct 7, 2018
@lemonsaurus lemonsaurus mentioned this pull request Oct 7, 2018
@jodth07 jodth07 mentioned this pull request Oct 25, 2018
3 tasks
@hedyhli hedyhli mentioned this pull request Oct 20, 2021
4 tasks
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.

4 participants