Expose node_id which is the entity ID used by v4 github api

cuedo / github-webhooks

Haskell types and instances for decoding GitHub webhook payloads.

https://hackage.haskell.org/package/github-webhooks

MIT License

34 stars 15 forks source link

Expose node_id which is the entity ID used by v4 github api #28

Closed AlistairB closed 4 years ago

AlistairB commented 4 years ago

Issue reference: Closes https://github.com/onrock-eng/github-webhooks/issues/27

Submission Checklist:

[x] Have you followed the guidelines in our Contributing document (for example, is your tree a clean merge)?
[x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
[x] Does your submission build?
[x] Does your submission pass tests?
[x] Have you run lints on your code locally prior to submission?
[x] Have you updated all of the cabal/nix infrastructure?
[x] Is this a breaking change? Have you discussed this?

kvanbere commented 4 years ago

Do you think that there is a case to suggest that the node_id should be optional ?

AlistairB commented 4 years ago

AFAICT, it is always there for certain entities and never there for others. Specifically these entities should always have it https://developer.github.com/v4/interface/node/

PageBuild for example is only exposed via v3 api it seems and as such does not have it.

All the example payloads in https://developer.github.com/v3/activity/events/types/ reliably appear to include it for certain entity types, but not for others.

As such I'm inclined to only add it to the entities that should have it and make it mandatory.

AlistairB commented 4 years ago

Personally, I like to err on the side of the more restrictive type that should be correct, as it will blow up loudly if the assumption is wrong. However, with the more permissive optional type, it may be mandatory but we may never notice. Happy to do whatever though :)

AlistairB commented 4 years ago

I made it optional for HookIssueLabels which currently has an optional id

kvanbere commented 4 years ago

Sounds good

AlistairB commented 4 years ago

@donkeybonks this should be good to go. I made some of the label related types to have the optional node_id just in case. The others I am pretty confident will always have it. Let me know if any changes are needed. Thanks!

kvanbere commented 4 years ago

Thanks, I'll review within the next 24hr.

kvanbere commented 4 years ago

Sorry, I'll need another 24hr.

AlistairB commented 4 years ago

No worries. No rush!

kvanbere commented 4 years ago

Looks good to me! Thanks.