Skip to content

handle_auth_response: wrong secret and raise error#51

Merged
dgw merged 1 commit intosopel-irc:masterfrom
bbuccianti:fix-hooks
Dec 4, 2019
Merged

handle_auth_response: wrong secret and raise error#51
dgw merged 1 commit intosopel-irc:masterfrom
bbuccianti:fix-hooks

Conversation

@bbuccianti
Copy link
Copy Markdown
Contributor

I'm just a student trying to learn so I really want your feedback guys!

Thanks a lot!

@dgw dgw added the duplicate label Dec 4, 2019
@dgw
Copy link
Copy Markdown
Member

dgw commented Dec 4, 2019

Definitely appreciate the initiative! However we already have more comprehensive changes to the database stuff being worked on in #49.

That said, if you want to extract your changes to the webhook handle_auth_response() function, those look good to have (and aren't covered by any pending PRs, to my knowledge). You could even just modify this PR branch and reword the commit message/PR title—ask for help on IRC if you need it. 😸

@bbuccianti bbuccianti changed the title sopel-github: module is working again! handle_auth_response: wrong secret and raise error Dec 4, 2019
Copy link
Copy Markdown
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I see you've already learned how to force-push branches—that's a useful skill! Are you sure you're "just a student"? 😛 I have only minor comments.

Comment thread sopel_modules/github/webhook.py Outdated
Comment thread sopel_modules/github/webhook.py Outdated
@dgw dgw added this to the 0.2.5 milestone Dec 4, 2019
Copy link
Copy Markdown
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

🎉

@dgw dgw merged commit 24e7e77 into sopel-irc:master Dec 4, 2019
@dgw dgw mentioned this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants