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

Maintain a manifest of generated files #234

Merged
merged 4 commits into from
Feb 12, 2017
Merged

Maintain a manifest of generated files #234

merged 4 commits into from
Feb 12, 2017

Conversation

jparise
Copy link
Collaborator

@jparise jparise commented Feb 11, 2017

This is signifiant rewrite of the compiler task to support manifests,
making us a better mix citizen and adding support for mix clean-based
file cleanup.

Our manifest is written as a binary-encoded Erlang term to give us the
most forward-compatible file format. At the moment, we only store a
manifest version number and the list of generated files, but we may
expand this in the future to include additional dependency information
such as the "version" of the code that was used to generate these files.
See #138 for a more detailed discussion.

This is signifiant rewrite of the compiler task to support manifests,
making us a better mix citizen and adding support for `mix clean`-based
file cleanup.

Our manifest is written as a binary-encoded Erlang term to give us the
most forward-compatible file format. At the moment, we only store a
manifest version number and the list of generated files, but we may
expand this in the future to include additional dependency information
such as the "version" of the code that was used to generate these files.
See #138 for a more detailed discussion.
end

@spec parse(Path.t, OptionParser.parsed) :: FileGroup.t
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why add @specs to private functions? It doesn't actively hurt anything but it is a maintenance burden... just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ironically, I added these when I played with moving some logic into another module, where these functions were public. I continued to find them useful as a way to document the internal arguments.

Copy link
Collaborator

@dantswain dantswain left a comment

Choose a reason for hiding this comment

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

lol I started digging around the mix source to see how they handle this and notice you've also been doing your homework @jparise ;)

It really seems like there should be a Mix.CompileTask behaviour or something... I'm still not clear which parts of this hook in to the compiler chain automatically... I assume if one calls mix clean then mix iterates over the compiler tasks and calls clean on each?

The only other feedback I have, and it's a take-it-or-leave-it kind of thing, would be to create a module that encapsulates dealing with the manifest. Might be overkill.

@jparise
Copy link
Collaborator Author

jparise commented Feb 11, 2017

Yeah, I spent a lot of time reading through mix to best understand how tasks like this should work.

  • mix clean calls all available clean functions on the modules listed in Mix.compilers.
  • mix compile calls all available manifests functions on all compile.* tasks.

Also, I added @recursive true to our compile task, which makes it run recursively when called as part of an umbrella project.

I ultimately decided not to introduce another module because the manifest bits are specific to this task and don't need to be part of our Thrift.Generator code. And reading the updated task end-to-end isn't too complicated, even with these changes.

@sourcelevel-bot
Copy link

Ebert has finished reviewing this Pull Request and has found:

  • 2 fixed issues! 🎉

But beware that this branch is 14 commits behind the pinterest:thrift_tng branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/pinterest/elixir-thrift/pulls/234.

@jparise jparise merged commit 359d70a into pinterest:thrift_tng Feb 12, 2017
@jparise jparise deleted the compile-manifest branch February 12, 2017 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants