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 Gitub Category #66

Merged
merged 28 commits into from
Jan 18, 2021
Merged

Add Gitub Category #66

merged 28 commits into from
Jan 18, 2021

Conversation

Shivansh-007
Copy link
Contributor

No description provided.

@Shivansh-007
Copy link
Contributor Author

Closing #11

@vivax3794 vivax3794 added area: commands Commands, cogs and extensions type: feature A request for or implementation of a new feature labels Jan 6, 2021
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.

Heyo, Shivansh 🥒 ! I haven't reviewed the entire PR yet, but here are some things you could look into.

bot/constants.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Show resolved Hide resolved
@Shivansh-007
Copy link
Contributor Author

Fixed merged conflict

@Shivansh-007
Copy link
Contributor Author

@codephile1221 Ok , thanks for the review, will mention those changes in the next commit.

@vivekashok1221
Copy link
Member

I wouldn't mind if you make multiple commits for the changes.
Similar changes can be in one commit.

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.

hello!
Few things 🌟

bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Show resolved Hide resolved
bot/exts/github/_issues.py Outdated 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.

One more thing which I wanted to add...

bot/exts/github/_source.py Outdated 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.

Hey! I'm done with the review of the whole PR ⭐ .
Here are some things we could discuss.

bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/_issues.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/_source.py Outdated Show resolved Hide resolved
bot/exts/github/_source.py Outdated Show resolved Hide resolved
bot/exts/github/_source.py Outdated Show resolved Hide resolved
bot/exts/github/_source.py Outdated Show resolved Hide resolved
1. Use MissingReqiuredArguments and let the error handler handle it.
2. Take channel as argument in issues helper instead of the id.
3. Remove redudant code.
4. Fix source link.
5. Remove the docstrings while sending code.
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.

Hi. Small things we could go through:

bot/exts/github/_source.py Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
1. Change issue usage to >user>/<repo>
2. Add better docstring for class Github.
3. Remove redundant code like instead of raising error if none, make it a required argument.
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.

Review as requested 😄 .

bot/exts/github/github.py Outdated Show resolved Hide resolved
bot/exts/github/github.py Outdated Show resolved Hide resolved
@Shivansh-007
Copy link
Contributor Author

Done.

bot/exts/github/github.py Outdated Show resolved Hide resolved
Co-authored-by: Vivek Ashok <[email protected]>
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 👍 !!!

Copy link
Contributor

@uncomfyhalomacro uncomfyhalomacro 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 to me. 💯

@Akarys42 Akarys42 merged commit 173f6bc into gurkult:main Jan 18, 2021
@Shivansh-007 Shivansh-007 deleted the feature/github-category branch February 7, 2021 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: commands Commands, cogs and extensions type: feature A request for or implementation of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants