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

A new plugin to post a welcome comment #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Cadair
Copy link
Member

@Cadair Cadair commented Jan 14, 2020

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.

@codecov-io
Copy link

Codecov Report

Merging #94 into master will decrease coverage by 0.94%.
The diff coverage is 21.42%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
baldrick/plugins/github_welcome.py 0% <0%> (ø)
baldrick/plugins/github_towncrier_changelog.py 79.76% <100%> ( 0.24%) ⬆️
baldrick/plugins/github_milestones.py 100% <100%> (ø) ⬆️
baldrick/plugins/github_pull_requests.py 83.51% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 116546d...c3247fc. Read the comment docs.

@@ -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)
Copy link
Contributor

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?

Copy link
Member Author

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...

Copy link
Contributor

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):
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Collaborator

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

Copy link
Collaborator

@astrofrog astrofrog 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! Can you add a changelog entry though?

@Cadair
Copy link
Member Author

Cadair commented Jan 15, 2020

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.

@pllim
Copy link
Contributor

pllim commented Oct 30, 2020

Maaaaaybe this should be GitHub Action...

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