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

examples/code_regen, but built in to goagen #1246

Merged
merged 7 commits into from Jun 6, 2017

Conversation

Projects
None yet
2 participants
@iancmcc
Copy link
Contributor

iancmcc commented Jun 4, 2017

This PR adds a --regen arg to goagen main and goagen controller, which has the same effect as the code_regen example, but without an external script or Makefile. It works fine with code generated using existing goagen (that is, uses the same // Controller_action: start_implement indicator).

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Jun 5, 2017

Thank you for doing this, looks great at first glance! I need to spend a bit more time reviewing it. In the mean time I was wondering if you could add documentation about this new flag to the website, specifically in this file: https://github.com/goadesign/goa.design/blob/master/content/implement/goagen.md? It would be helpful if the doc described a little bit the expected workflow (e.g. first use bootstrap then after the design is updated run regen to re-generate the controllers. For this to work the comments ... start ... and ... end ... need to be kept etc. )

@iancmcc

This comment has been minimized.

Copy link
Contributor

iancmcc commented Jun 5, 2017

Creating a PR for that now. In the meantime, there are two areas of this PR that I'm not super happy with, and perhaps you have suggestions.

  • Imports - the regenerated files have the default imports, the one part of the implemented controllers that can't (currently) get saved. goimports fixes it immediately, of course, but a goagen controller --regen followed immediately by a go build will likely fail due to missing imports. Not ideal. Could conceivably just always save imports.

  • Failures - in one case, specifically where the existing files are read in to extract the controller impls, a failure would (currently) result in the files getting wiped, since an empty map of extracted controllers is returned. Would a log.Fatal or equivalent there be appropriate?

Thanks!

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Jun 5, 2017

Thanks for raising the issues.

  • Wrt import it should be possible to leverage the Go stdlib astutil package to parse the existing imports and "add them back". As an example https://github.com/goadesign/goa/blob/master/goagen/codegen/workspace.go#L250 uses the package to remove imports that are not used.

  • Wrt failures maybe I'm missing something but if the generator returned an error instead of writing the empty map then wouldn't that just do what we need?

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Jun 6, 2017

The changes and the PR look great, thank you! It would be nice to be able to preserve the import statements though - do you have a handle on how to do that?

@iancmcc

This comment has been minimized.

Copy link
Contributor

iancmcc commented Jun 6, 2017

There! Returning an error was, yeah, simple; somehow I had forgotten that extractControllerBody was a function I had literally just introduced and I was therefore wary of changing the signature. Chalk it up to lack of sleep.

Imports were simple once you pointed me there, thanks. Trimming the quotes is a little hacky, but better that than rewriting codegen or double-processing the file with ast.

Still working on the website update.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Jun 6, 2017

Looks great! Could I coerce you into writing a test for the import preserving logic? I'll merge once that's done, thanks again.

@iancmcc

This comment has been minimized.

Copy link
Contributor

iancmcc commented Jun 6, 2017

Tests added for the regeneration, the behavior of regen when the files don't exist yet, and the import preservation. Also, technically, controller generation in the first place, although that may be covered elsewhere, but it made the tests flow better.

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Jun 6, 2017

Looks great, thank you. OK one last thing :) do you think you could backport (cherry-pick) this PR onto the v1 branch?

@raphael raphael merged commit 7a9154a into goadesign:master Jun 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

iancmcc added a commit to iancmcc/goa that referenced this pull request Jun 6, 2017

examples/code_regen, but built in to goagen (goadesign#1246)
Add `--regen` goagen flag for `main` and `controller` commands.

raphael added a commit that referenced this pull request Jun 6, 2017

examples/code_regen, but built in to goagen (#1246) (#1249)
Add `--regen` goagen flag for `main` and `controller` commands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment