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

Initial drop of generated types #2

Merged
merged 7 commits into from
Oct 29, 2021

Conversation

magnusbaeck
Copy link
Member

Applicable Issues

N/A; initial drop of code

Description of the Change

This is intended to be a first usable attempt at a Go SDK for Eiffel events. It covers the events from the Lyon edition.

The principle chosen is to have one Go struct per major version of each event. This means that v3.0.0 of some event received over the wire might be deserialized into a struct that supports up to v3.3.0. That version defines additional fields, which will obviously be empty. Similarly, if you instantiate the V3 struct for that event type you need to be careful about which struct fields you set if you don't intend to use the most recent version within that major version.

Non-exhaustive list of missing features:

  • Factory function for each struct that populates basic required fields like meta.type, meta.version, meta.id, and meta.time.
  • JSON serialization that (optionally) includes validation.
  • Subpackages for all Eiffel editions that define type aliases that point to concrete structs with major versions matching the contents of the edition.

The structs and other types are generated based on the schemas. For that reason the Eiffel protocol repository is included as a Git submodule.

Alternate Designs

None.

Benefits

Better Eiffel experience for gophers.

Possible Drawbacks

None.

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

(b) The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

(c) The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

(d) I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Signed-off-by: Magnus Bäck <magnus.back@axis.com>

This is intended to be a first usable attempt at a Go SDK for Eiffel
events. It covers the events from the Lyon edition.

The principle chosen is to have one Go struct per major version of each
event. This means that v3.0.0 of some event received over the wire might
be deserialized into a struct that supports up to v3.3.0. That version
defines additional fields, which will obviously be empty. Similarly, if
you instantiate the V3 struct for that event type you need to be careful
about which struct fields you set if you don't intend to use the most
recent version within that major version.

Non-exhaustive list of missing features:

    - Factory function for each struct that populates basic required
      fields like meta.type, meta.version, meta.id, and meta.time.
    - JSON serialization that (optionally) includes validation.
    - Subpackages for all Eiffel editions that define type aliases
      that point to concrete structs with major versions matching
      the contents of the edition.

The structs and other types are generated based on the schemas. For that
reason the Eiffel protocol repository is included as a Git submodule.
@magnusbaeck magnusbaeck marked this pull request as ready for review October 19, 2021 21:20
@magnusbaeck magnusbaeck requested a review from a team as a code owner October 19, 2021 21:20
@magnusbaeck
Copy link
Member Author

Sorry for the huge PR but most of the code has been autogenerated so it's not really that huge.

Is it expected that GitHub Actions aren't run based on the contents of the PR? I.e. you actually have to submit your workflow to the branch before they're run?

@t-persson
Copy link

Sorry for the huge PR but most of the code has been autogenerated so it's not really that huge.

Is it expected that GitHub Actions aren't run based on the contents of the PR? I.e. you actually have to submit your workflow to the branch before they're run?

The Github actions will run when the workflows exist in the repository. If you had done your changes on "magnusbaeck:master" then they would've run there. Or if you don't restrict the workflow to "master", then it will run on all branches on your fork as well

@magnusbaeck
Copy link
Member Author

The Github actions will run when the workflows exist in the repository. If you had done your changes on "magnusbaeck:master" then they would've run there. Or if you don't restrict the workflow to "master", then it will run on all branches on your fork as well

Okay, so if the workflow exists in the first place it'll run for subsequent PRs? IOW, it's really only a problem when introducing new workflows?

Anyway, I pushed to magnusbaeck:master and the workflow ran (initially with an error that I've since fixed).

For your reviewing pleasure I'll try the GitHub way of pushing extra commits on the PR branch instead of amending, but this requires some extra care when merging to avoid a garbage commits message in the squashed commit.

Copy link

@t-persson t-persson left a comment

Choose a reason for hiding this comment

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

Looks complicated, but good :)

I only looked at one of the generated event files. But I have not verified that they are correctly generated.

Would also like someone more than me to approve this PR since it's so much.

events_test_roundtripdata.go Outdated Show resolved Hide resolved
internal/cmd/eventgen/eventtypes.go Outdated Show resolved Hide resolved
Copy link

@felixhall felixhall left a comment

Choose a reason for hiding this comment

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

Looks great, I have no objections, but please don't ask me to explain the details involved to anyone :D

Now that eiffel-community/eiffel#284 has been
merged all the examples in the protocol git work (and we obviously have
to update the submodule link to point to that commit).
@@ -0,0 +1,58 @@
// Copyright Axis Communications AB.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should this file be moved into a testdata/ folder perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

The compiler ignores testdata directories so we'd have to turn it into a JSON or YAML file. We could consider moving this file along with events_test.go to a subpackage, at least if further changes to this package requires additional testdata source files etc. Right now it seems like overkill with a subpackage.

internal/cmd/eventgen/enum.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
unmarshal.go Outdated Show resolved Hide resolved
@magnusbaeck magnusbaeck merged commit 690caf3 into eiffel-community:master Oct 29, 2021
@magnusbaeck magnusbaeck deleted the make-usable branch October 29, 2021 10:08
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.

None yet

4 participants