-
Notifications
You must be signed in to change notification settings - Fork 8
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
A new plugin to post a welcome comment #94
base: master
Are you sure you want to change the base?
Conversation
aa8461b
to
9ab5956
Compare
02c8268
to
c3247fc
Compare
Codecov Report
@@ Coverage Diff @@
## master #94 /- ##
=========================================
- Coverage 82.34% 81.4% -0.95%
=========================================
Files 17 18 1
Lines 912 925 13
=========================================
Hits 751 753 2
- Misses 161 172 11
Continue to review full report at Codecov.
|
@@ -138,7 138,7 @@ def process_pull_request(repository, number, installation, action, | |||
results = {} | |||
for function, actions in PULL_REQUEST_CHECKS.items(): | |||
if actions is None or action in actions: | |||
result = function(pr_handler, repo_handler) | |||
result = function(pr_handler, repo_handler, payload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to break backward compat? Can this be an optional keyword as above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a function sig, we are calling the registered callback functions. The only way to pass this information is to always pass it. We could do some crazy signature introspection stuff but...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see your point... 🤔
|
||
|
||
def process_pull_request(repository, number, installation, action, | ||
is_new=False): | ||
is_new=False, *, payload): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean payload
has to be a keyword? If so, why is no default assigned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because I didn't want to change the signature, but we always should pass payload in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other change is already a breaking change, so why not just change the signature here also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think it makes sense like this though, as a required keyword argument
There was a problem hiding this 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! Can you add a changelog entry though?
I think there is actually a better way to achieve this, without the breaking change. However, I do wonder if passing the full webhook payload through is still a good idea. |
Maaaaaybe this should be GitHub Action... |
This just posts a comment on all PRs only on open events. It might be nice to be able to limit this to new contributors, or exclude people who are in teams in the org or something. However, for now this should suffice.
Based on #87 and needs docs.