feat: support binding to generated package #285

costela · 2023-07-18T07:37:17Z

This allows one to generate code in an existing package without causing import loops.

It is now possible to use something like the following (assuming we're in module github.com/foo/bar):

generated: generated/genqlient.go
package_bindings:
  - github.com/foo/bar/generated

or (shorthand variant of the example above)

generated: generated/genqlient.go
package_bindings:
  - .

or

generated: generated/genqlient.go
bindings:
  Bar:
    type: github.com/foo/bar/generated.Bar

The last example assumes some file in generated/ defines the type Bar.

Closes #283

benjaminjkraft

Cool, thanks for figuring this out! I'm very happy with how little code it turned out to be, and the tests are very convincing that it works.

One question I have is whether we should allow any valid relative path in the genqlient.yaml, i.e. you could say ./mypkg/subpkg if that's what you want. That seems like it could be quite convenient rather than writing out the name of the current package a bunch of times. In that case we'd probably want to say . means the directory with the genqlient.yaml. Although, maybe it's better to say you have to write it out, like you do in Go import blocks. In fact I think these changes would solve the problem without allowing special . syntax at all -- you would just do what you did in the first two test cases. I think I'd prefer to either fully support the usual, or stick to what we have, since config syntax is always confusing and can be more annoying to change later.

Beyond that I think my questions are about whether running this in the real world could hit weird cases we don't really see in a test. I put some comments inline. But probably if we can't figure those out we can just see if anyone complains.

benjaminjkraft · 2023-07-18T17:00:40Z

generate/config.go

+	if err != nil {
+		return fmt.Errorf("loading generated package: %w", err)
+	}


I wonder if this is safe. Like would this break things if the directory where you're putting the output is empty (as it would be if your output is in a separate package and this is the first run of genqlient)? On the other hand swallowing the error seems dangerous, and it's not clear how easy it is to know if we actually need packagePath. Maybe instead we should just look at baseDir and then add the suffix ourselves? (Is that safe?)

(For that matter, does this require that baseDir is a valid Go package? Or at least one with at least one Go file with valid package clause? Maybe that's fine; if you have some kind of monorepo where all the Go is in a subdirectory you can just put genqlient.yaml in that directory.)

If we don't know the answer, it's probably not a huge deal; it doesn't really affect the API surface we are committing to so if we break someone's workflow we can fix it then.

would this break things if the directory where you're putting the output is empty

Yes, it would break, but that would be the same as trying to bind to an empty package, right?

does this require that baseDir is a valid Go package?

It shouldn't: it's just used as a base for the package search.

benjaminjkraft · 2023-07-18T17:02:30Z

generate/config.go

@@ -203,6 +223,7 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {

 			mode := packages.NeedDeps | packages.NeedTypes
 			pkgs, err := packages.Load(&packages.Config{
+				Dir:  c.baseDir,


Thinking out loud: this should be ok for third-party packages because it's just the directory where we run the build tool, so if binding.Package is absolute then it'll still return the right package (and, if relevant, will now respect any versions/replaces in the module, if they differ from those in the current directory, which is probably actually better than current behavior, in the rare case where it matters).

yes, exactly. This is just needed to support the "relative import" hack we use above, which isn't usual, but is supported.

benjaminjkraft · 2023-07-19T03:25:34Z

generate/generate_test.go

@@ -21,24 +21,41 @@ const (
 // buildGoFile returns an error if the given Go code is not valid.
 //
 // namePrefix is used for the temp-file, and is just for debugging.
-func buildGoFile(namePrefix string, content []byte) error {
+func buildGoFile(namePrefix string, content []byte, extraFiles ...string) error {


benjaminjkraft · 2023-07-19T03:38:00Z

docs/genqlient.yaml

+#
+# Using "." is a shorthand for the package containing the generated file.
+# Both allow the generated code to live in the same package as its types.


FWIW, I would have some concerns about actually using the generated package here. I worry it would end up being circular in ways that are hard to understand -- in that now genqlient depends on genqlient-generated types. I don't think we need to ban it, but maybe we should say in the comment here that it's not a good idea? Or does it actually work fine?

Also, whatever we settle on for the syntax, can you add it to the docs for bindings above too?

now genqlient depends on genqlient-generated types

Yes, a warning might be appropriate, but it should work as long as the user avoids name conflicts.
I've added a "double generate" test to try to cover this case and made it more explicit in the comment.

can you add it to the docs for bindings above too

The shorthand is intentionally only supported in the "whole package" mode. Otherwise one would end up with ..ID or some other weird syntax to reference the types inside the package. Just didn't feel like it was worth it. WDYT?

This allows one to generate code in an existing package without causing import loops.

costela

Thanks for the feedback and sorry for the late reply! Vacation got in the way!

PTAL again! 🙏

costela · 2023-08-24T13:08:28Z

docs/genqlient.yaml

+#
+# Using "." is a shorthand for the package containing the generated file.
+# Both allow the generated code to live in the same package as its types.


now genqlient depends on genqlient-generated types

Yes, a warning might be appropriate, but it should work as long as the user avoids name conflicts.
I've added a "double generate" test to try to cover this case and made it more explicit in the comment.

can you add it to the docs for bindings above too

The shorthand is intentionally only supported in the "whole package" mode. Otherwise one would end up with ..ID or some other weird syntax to reference the types inside the package. Just didn't feel like it was worth it. WDYT?

costela · 2023-08-24T13:13:50Z

generate/config.go

+	if err != nil {
+		return fmt.Errorf("loading generated package: %w", err)
+	}


would this break things if the directory where you're putting the output is empty

Yes, it would break, but that would be the same as trying to bind to an empty package, right?

does this require that baseDir is a valid Go package?

It shouldn't: it's just used as a base for the package search.

costela · 2023-08-24T13:15:36Z

generate/config.go

@@ -203,6 +223,7 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error {

 			mode := packages.NeedDeps | packages.NeedTypes
 			pkgs, err := packages.Load(&packages.Config{
+				Dir:  c.baseDir,


yes, exactly. This is just needed to support the "relative import" hack we use above, which isn't usual, but is supported.

benjaminjkraft · 2024-02-18T02:19:30Z

Closing in favor of #316 which makes this as a narrower change to support existing syntax. If you're interested, you could revisit adding the shorthand (package_bindings: . etc.); I think doing that separately will make some of the still-questions about what syntax we should support clearer.

costela requested review from benjaminjkraft, csilvers, dnerdy and StevenACoffman as code owners July 18, 2023 07:37

costela mentioned this pull request Jul 18, 2023

Binding to a type in the same package as generated code produces invalid output #283

Closed

benjaminjkraft requested changes Jul 19, 2023

View reviewed changes

benjaminjkraft mentioned this pull request Aug 1, 2023

feat: allow imports from same generated package #289

Closed

6 tasks

feat: support binding to generated package

18cb77e

This allows one to generate code in an existing package without causing import loops.

costela force-pushed the leo.antunes/allow-generating-bindings-for-local-package branch from ae9f207 to 18cb77e Compare August 24, 2023 13:26

costela commented Aug 24, 2023

View reviewed changes

costela requested a review from benjaminjkraft August 24, 2023 13:28

StevenACoffman removed their request for review October 12, 2023 18:38

benjaminjkraft closed this Feb 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support binding to generated package #285

feat: support binding to generated package #285

costela commented Jul 18, 2023

benjaminjkraft left a comment

benjaminjkraft Jul 18, 2023

costela Aug 24, 2023

benjaminjkraft Jul 18, 2023

costela Aug 24, 2023

benjaminjkraft Jul 19, 2023

benjaminjkraft Jul 19, 2023

costela Aug 24, 2023

costela left a comment

costela Aug 24, 2023

costela Aug 24, 2023

costela Aug 24, 2023

benjaminjkraft commented Feb 18, 2024

feat: support binding to generated package #285

feat: support binding to generated package #285

Conversation

costela commented Jul 18, 2023

benjaminjkraft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminjkraft commented Feb 18, 2024