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

Fix issues related to introduction of new values in GHEvent #1159

Merged
merged 4 commits into from
Jun 1, 2021

Conversation

gsmet
Copy link
Contributor

@gsmet gsmet commented May 27, 2021

@bitwiseman I decided to apply the same strategy we used for other things: use Strings for serialization/deserialization and only switch to enums for APIs. It's a bit less optimal but less risky. Another better option would be to have custom Jackson deserializer but that's probably something we need to keep for 2.0.

I also fixed an inconsistency in EnumUtils: one method was user uppercase values and not the other. This is a recipe for disaster.

Better reviewed commit per commit as I kept them semantic.

I'm not exactly sure if it's worth adding tests as we would have to add tests with unknown values for each enum all over the place.

Fixes #1156

@codecov
Copy link

codecov bot commented May 27, 2021

Codecov Report

Merging #1159 (54d8fe9) into main (4abf33a) will increase coverage by 0.02%.
The diff coverage is 91.17%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1159      +/-   ##
============================================
+ Coverage     73.44%   73.47%   +0.02%     
- Complexity     1834     1839       +5     
============================================
  Files           185      185              
  Lines          6146     6148       +2     
  Branches        367      367              
============================================
+ Hits           4514     4517       +3     
+ Misses         1411     1410       -1     
  Partials        221      221              
Impacted Files Coverage Δ
src/main/java/org/kohsuke/github/GHHook.java 85.71% <0.00%> (+4.46%) ⬆️
src/main/java/org/kohsuke/github/GHApp.java 72.54% <75.00%> (+1.12%) ⬆️
...ain/java/org/kohsuke/github/GHAppInstallation.java 51.78% <75.00%> (+1.78%) ⬆️
src/main/java/org/kohsuke/github/GHEvent.java 100.00% <100.00%> (ø)
src/main/java/org/kohsuke/github/GHEventInfo.java 93.93% <100.00%> (+8.22%) ⬆️
...in/java/org/kohsuke/github/internal/EnumUtils.java 100.00% <100.00%> (ø)

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 4abf33a...54d8fe9. Read the comment docs.

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

The case sensitivity is intentional. The values coming from GitHub are Pascal-cased and I want to match them specifically or not at all.

Please just add the new events in this PR.

@gsmet
Copy link
Contributor Author

gsmet commented May 27, 2021

Mkay. My big concern is that we have two nearly identical methods and the nullable behavior should be the only difference between the two.

I don't mind how we fix it but I think we need to make the methods consistent in one way or another.

I will remove this commit from the PR.

@bitwiseman
Copy link
Member

@gsmet
You are completely correct in principle, and I'm almost sure you're right in this case.

I think the reason I chose to do it that way was to match the exiting method's behavior (which was case-sensitive). The events area is not well tested, so I would like to err on the side of caution. Perhaps not it as an issue and link to 38460dc and we can come back to it?

@gsmet gsmet force-pushed the check-run-enum branch from 9e9e2c4 to a79971e Compare May 27, 2021 11:44
@gsmet
Copy link
Contributor Author

gsmet commented May 27, 2021

OK, I removed this commit and did the case change at the call sites.

@hcoles
Copy link

hcoles commented Jun 1, 2021

Is there anything I can do to help get this merged?

@sathishavm
Copy link

sathishavm commented Jun 1, 2021

I like this change, can this merge?

We took a change that added an enum that was used purely for mapping from
EventInfo.type to GHEvent. This seemed fine but that enum is used only by EventInfo.

This change removed that enum and adds a map to EventInfo to do the required mapping.
This avoids shoehorning mapping behavior in to the EnumUtils.
@bitwiseman bitwiseman merged commit fbfba70 into hub4j:main Jun 1, 2021
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.

Checks api functionality broken by recent github api change
4 participants