search

mirage / ocaml-github

GitHub APIv3 OCaml bindings
ISC License
100 stars 61 forks source link

parse_event now also return the event metadata #180

Closed samoht closed 7 years ago

samoht commented 7 years ago

e.g. the events' repository, sender, org, etc

dsheets commented 7 years ago

The web hook event schema is a bit sad type-wise. Do you want to use the metadata generically?

Another option would be to have the web hook event payloads be a set of types which double inherit from the plain event types and the hook event metadata type. We already know of some events (like push) that vary from their stream type when delivered via web hook so this would just be an extension of that variation. We could also offer projection functions back to the generic metadata if you have a need for that. atdgen could generate these projections from the inheritance hierarchy as well as the current match in Hook.parse_event. This is clearly more effort than your current solution but also seems cleaner (parse once instead of twice) and like a nicer API. What do you think?

Finally, as we're already changing the API and this will trigger 3.0, could you rename all of the web hook event types and identifiers from *_event_hook (specialization order) to hook_event (English modifier order)? I'm not sure why I/we decided on the former but every time I read it, I find it confusing now.

Thanks for the PR!

samoht commented 7 years ago

Yes the types are not very nice, I am not sure how the inherit between records and variants is supposed to work (and when I tested it, it didn't). I will do the renaming.

In my use-case, I just need to know the repository from where that event comes from, as I receive it from an organisation hook.

dsheets commented 7 years ago

I'm not sure about inherit between records and variants but inherit between multiple record types is used pretty extensively. In my proposal, the return type for parse_event would be a variant and each constructor argument would be a double-inherited *_hook_event. There would then be some projections to produce the sub-types from the conglomerate type.

Do you need to know the repository for all event types? Or only after dispatching against the type of the event?

samoht commented 7 years ago

Do you need to know the repository for all event types? Or only after dispatching against the type of the event?

I parse both at the same so the order doesn't matter much, although it would be simpler if all *_hook_event values would contain the repository (and possibly the ID). I'll try to come-up with a better proposal.

dsheets commented 7 years ago

Could we just have a second function to parse the metadata so the limitation/reparsing is exposed to the user while we have no better implementation to hand? This would also be backward compatible.

dsheets commented 7 years ago

Appveyor is complaining about unzip and Fedora 24 has a problem building dose3.5.0.1 due to missing pod2html so I'm going to merge this regardless.

samoht commented 7 years ago

@dsheets the fix for appveyor is to use https://github.com/ocaml/ocaml-ci-scripts/blob/master/appveyor.yml

dsheets commented 7 years ago

Thanks for the tip. I've updated it in #188.