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

Remove dependency on github.com/2opremio/go-1/codec #973

Closed
2opremio opened this issue Feb 17, 2016 · 5 comments · Fixed by #980
Closed

Remove dependency on github.com/2opremio/go-1/codec #973

2opremio opened this issue Feb 17, 2016 · 5 comments · Fixed by #980
Labels
tech-debt Unpleasantness that does (or may in future) affect development
Milestone

Comments

@2opremio
Copy link
Contributor

As a followup on #916 (comment)

@2opremio 2opremio added the tech-debt Unpleasantness that does (or may in future) affect development label Feb 17, 2016
@2opremio 2opremio mentioned this issue Feb 17, 2016
4 tasks
@2opremio
Copy link
Contributor Author

@ugorji Has very efficiently solved all the bugs I reported and changed the selfer-generation behaviour of codecgen so that we don't need the // codecgen: skip of github.com/2opremio/go-1/codec.

In particular he has implemented "do not create Selfer for types which already have implemented" (ugorji/go@5d64d76), which is great but, due to the detection being reflection-based, it can break serialization of embedded-types if the code-generation is not done in a careful order. See ugorji/go#141 (comment)

The solutions to this are:

  • Being super careful about the code-generation order, always removing all the previously generated files before re-generating any of them.
  • Stop using embedded types in our code.

@peterbourgon , @paulbellamy Thoughts?

@peterbourgon
Copy link
Contributor

I wish I had caught the introduction of embedding in JSON-serialized types at PR review, I would have flagged it. tl;dr: it is a category error to use embedding in types that will be JSON serialized (AFAIK this was an invariant prior to the controls work) and we should absolutely eliminate embedding in types that will be serialized.

@2opremio
Copy link
Contributor Author

I agree. That's also the solution @ugorji was implicitly suggesting in ugorji/go#141

@2opremio
Copy link
Contributor Author

Alright, I will:

  • Bump ugorji to get the fixes
  • Remove the embeddings (I can't still believe how broken Golang is in this respect)
  • Remove github.com/2opremio/go-1/codec

@peterbourgon Sounds good?

@2opremio
Copy link
Contributor Author

Also, Golang doesn't seem to have any tags to inline JSON (e.g. Jackson's @JsonUnwrapped), so removing the embedding will change the JSON schema and will require changes in the UI.

@foot Would you mind helping me out with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Unpleasantness that does (or may in future) affect development
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants