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

Use the embed package to do less in mktemplates. #228

Merged
merged 2 commits into from
Apr 16, 2022

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Mar 26, 2022

...with a bit more work we can probably get rid of the mktemplates
command entirely, but this was low hanging fruit.

Note that this also re-generates some of the .capnp.go files. The
differences are I assume because I am using a new version of the capnp
compiler than when these were last generated? It seems nothing changed
except for the exact bytes of the schema; the actual generated code
is the same. We really should have CI make sure these are correct somehow.

@zenhack
Copy link
Contributor Author

zenhack commented Mar 26, 2022

Update, went ahead and finished the job; mktemplates is gone.

@lthibault
Copy link
Collaborator

lthibault commented Mar 27, 2022

This looks promising 🙂 I'm going to be traveling next week, and will have limited availability, but I'll give this a thorough review when I get back. Feel free to merge if it's blocking you in the meantime.

@lthibault
Copy link
Collaborator

We should probably change this import to "context".

Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

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

LGTM overall. See inline comment regarding context package.

@zenhack
Copy link
Contributor Author

zenhack commented Apr 16, 2022

I am confused; I don't see an import statement at that link?

@lthibault
Copy link
Collaborator

I am confused; I don't see an import statement at that link?

Oh, strange. It's line 36: contextImport = "golang.org/x/net/context".

...with a bit more work we can probably get rid of the mktemplates
command entirely, but this was low hanging fruit.

Note that this also re-generates some of the .capnp.go files. The
differences are I assume because I am using a new version of the capnp
compiler than when these were last generated? It seems nothing changed
except for the exact bytes of the schema; the actual generated *code*
is the same. We really should have CI make sure these are correct somehow.
@zenhack
Copy link
Contributor Author

zenhack commented Apr 16, 2022

Rebased on top of master to fix the merge conflicts. That should also pull in the change in #227, which deals with that import.

@zenhack zenhack merged commit 47559d5 into capnproto:main Apr 16, 2022
@zenhack zenhack deleted the embed-package branch April 16, 2022 23:22
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.

2 participants