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 enums for webhook payloads action field #3136

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

Conversation

prnvbn
Copy link
Contributor

@prnvbn prnvbn commented Apr 21, 2024

Relevant Issue: #3127

Copy link

google-cla bot commented Apr 21, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

codecov bot commented Apr 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.88%. Comparing base (2b8c7fa) to head (7fb9e96).
Report is 43 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3136      +/-   ##
==========================================
- Coverage   97.72%   92.88%   -4.84%     
==========================================
  Files         153      170      +17     
  Lines       13390    11426    -1964     
==========================================
- Hits        13085    10613    -2472     
- Misses        215      723     +508     
  Partials       90       90              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 21, 2024

Ah! Now I understand better the scope of this breaking API change.
I apologize that I couldn't quite visualize fully what we were discussing before.

I would like @willnorris to weigh in on this PR to get his thoughts on the value of this change vs the breaking nature of it (and therefore the impact on developers) before we proceed further.

@gmlewis gmlewis added waiting for reply Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Apr 21, 2024
@prnvbn
Copy link
Contributor Author

prnvbn commented Apr 22, 2024

Makes sense!

@prnvbn
Copy link
Contributor Author

prnvbn commented Apr 28, 2024

@gmlewis @willnorris any update on the PR?

@prnvbn
Copy link
Contributor Author

prnvbn commented Aug 30, 2024

this can be done progressively as well if you prefer and do it one event at a time (or in batches)

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 30, 2024

this can be done progressively as well if you prefer and do it one event at a time (or in batches)

If we're going to proceed, then I think one huge breaking change like this is probably better than a bunch of smaller changes.

But I would still like to hear what @willnorris thinks about changing all the *strings into explicit enum values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). waiting for reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants