go-github icon indicating copy to clipboard operation
go-github copied to clipboard

Add enums for webhook payloads action field

Open prnvbn opened this issue 1 year ago • 7 comments

Relevant Issue: https://github.com/google/go-github/issues/3127

prnvbn avatar Apr 21 '24 08:04 prnvbn

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.

google-cla[bot] avatar Apr 21 '24 08:04 google-cla[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 92.88%. Comparing base (2b8c7fa) to head (7fb9e96). Report is 233 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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 21 '24 13:04 codecov[bot]

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 avatar Apr 21 '24 13:04 gmlewis

Makes sense!

prnvbn avatar Apr 22 '24 17:04 prnvbn

@gmlewis @willnorris any update on the PR?

prnvbn avatar Apr 28 '24 09:04 prnvbn

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

prnvbn avatar Aug 30 '24 09:08 prnvbn

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.

gmlewis avatar Aug 30 '24 12:08 gmlewis

@willnorris - do you have any opinions about a large breaking change like this to change a bunch of *strings into explicit enum values?

gmlewis avatar Jan 22 '25 01:01 gmlewis

Would be interested to know if there is a decision taken about this. I personally would prefer enums as they are self documenting the possibilities a value can have.

prnvbn avatar Jan 22 '25 01:01 prnvbn

@willnorris - do you have any opinions about a large breaking change like this to change a bunch of *strings into explicit enum values?

Sorry I missed your question back in August. Personally, I'm not a fan. The surface area of the library is already huge, so I'd be pretty sensitive to changes like this that increase it all the more. 🤷🏻

willnorris avatar Jan 22 '25 04:01 willnorris

@willnorris - do you have any opinions about a large breaking change like this to change a bunch of *strings into explicit enum values?

Sorry I missed your question back in August. Personally, I'm not a fan. The surface area of the library is already huge, so I'd be pretty sensitive to changes like this that increase it all the more. 🤷🏻

Thank you, @willnorris - and thank you, @prnvbn for your patience. I will close this PR as we will not be pursuing this change.

gmlewis avatar Jan 22 '25 12:01 gmlewis