-
Notifications
You must be signed in to change notification settings - Fork 103
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
Improve package-sniffing and bind correctly to types in the same package #316
Changes from 5 commits
6e70eb1
6549706
852b48c
a9dba06
5190f51
83d2bab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,8 @@ type Config struct { | |
// The directory of the config-file (relative to which all the other paths | ||
// are resolved). Set by ValidateAndFillDefaults. | ||
baseDir string | ||
// The package-path into which we are generating. | ||
pkgPath string | ||
} | ||
|
||
// A TypeBinding represents a Go type to which genqlient will bind a particular | ||
|
@@ -132,6 +134,46 @@ func pathJoin(a, b string) string { | |
return filepath.Join(a, b) | ||
} | ||
|
||
func (c *Config) getPackageNameAndPath() (pkgName, pkgPath string, err error) { | ||
abs, err := filepath.Abs(c.Generated) | ||
if err != nil { | ||
return "", "", err | ||
} | ||
|
||
dir := filepath.Dir(abs) | ||
pkgNameGuess := filepath.Base(dir) | ||
if !token.IsIdentifier(pkgNameGuess) { | ||
pkgNameGuess = "" | ||
} | ||
|
||
pkgs, err := packages.Load(&packages.Config{Mode: packages.NeedName}, dir) | ||
if err != nil { | ||
return pkgNameGuess, "", err | ||
} else if len(pkgs) != 1 { | ||
return pkgNameGuess, "", fmt.Errorf("found %v packages in %v, expected 1", len(pkgs), dir) | ||
} | ||
|
||
pkg := pkgs[0] | ||
// TODO(benkraft): Can PkgPath ever be empty without error? If so, we could | ||
// warn. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't know the semantics of packages.Load, but it seems like Name is empty if the package doesn't exist, but PkgPath is still set in that case? |
||
if pkg.Name != "" { | ||
return pkg.Name, pkg.PkgPath, nil | ||
} | ||
|
||
// e.g. empty package yet to be created, see if we can just guess a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, this comment is very helpful! |
||
// reasonable name. | ||
pathSuffix := filepath.Base(pkg.PkgPath) | ||
if token.IsIdentifier(pathSuffix) { | ||
pkgNameGuess = pathSuffix | ||
} | ||
|
||
if pkgNameGuess != "" { | ||
return pkgNameGuess, pkg.PkgPath, nil | ||
} else { | ||
return "", "", fmt.Errorf("no package found in %v", dir) | ||
} | ||
} | ||
|
||
// ValidateAndFillDefaults ensures that the configuration is valid, and fills | ||
// in any options that were unspecified. | ||
// | ||
|
@@ -167,29 +209,42 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error { | |
"\nExample: \"github.com/Org/Repo/optional.Value\"") | ||
} | ||
|
||
if c.Package != "" { | ||
if !token.IsIdentifier(c.Package) { | ||
// No need for link here -- if you're already setting the package | ||
// you know where to set the package. | ||
return errorf(nil, "invalid package in genqlient.yaml: '%v' is not a valid identifier", c.Package) | ||
} | ||
} else { | ||
abs, err := filepath.Abs(c.Generated) | ||
if err != nil { | ||
return errorf(nil, "unable to guess package-name: %v"+ | ||
"\nSet package name in genqlient.yaml"+ | ||
"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", err) | ||
} | ||
if c.Package != "" && !token.IsIdentifier(c.Package) { | ||
// No need for link here -- if you're already setting the package | ||
// you know where to set the package. | ||
return errorf(nil, "invalid package in genqlient.yaml: '%v' is not a valid identifier", c.Package) | ||
} | ||
|
||
base := filepath.Base(filepath.Dir(abs)) | ||
if !token.IsIdentifier(base) { | ||
return errorf(nil, "unable to guess package-name: '%v' is not a valid identifier"+ | ||
"\nSet package name in genqlient.yaml"+ | ||
"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", base) | ||
pkgName, pkgPath, err := c.getPackageNameAndPath() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to comment that this ignores c.Package, and that below you'll check to make sure they're in agreement (or, at least, not in disagreement.) |
||
if err == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It took me a while to figure out what was going on here, because I'm so used to the Go convention of checking for the error case in the if's, not the success case. I wonder if there's a way to reorganize this code to keep the current semantics but perhaps adhere more closely to Go convention. Maybe not. |
||
if c.Package == pkgName || c.Package == "" { | ||
c.Package = pkgName | ||
} else { | ||
warn(errorf(nil, "warning: package setting in genqlient.yaml '%v' looks wrong "+ | ||
"('%v' is in package '%v') but proceeding with '%v' anyway\n", | ||
c.Package, c.Generated, pkgName, c.Package)) | ||
} | ||
|
||
c.Package = base | ||
} else if c.Package != "" { | ||
// If you specified a valid package, at least try to use that. | ||
// But we can't set pkgPath, which means you'll run into trouble | ||
// binding against the generated package, so at least warn. | ||
warn(errorf(nil, "warning: unable to identify current package-path "+ | ||
"(using 'package' config '%v'): %v\n", c.Package, err)) | ||
} else if pkgName != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unusual, though I guess not unheard of, for the value of the other return-values to matter when |
||
// If the directory-name is valid, use that. This is useful if you | ||
// somehow can't build, and especially for tests. | ||
warn(errorf(nil, "warning: unable to identify current package-path "+ | ||
"(using directory name '%v': %v\n", pkgName, err)) | ||
c.Package = pkgName | ||
} else { | ||
return errorf(nil, "unable to guess package-name: %v"+ | ||
"\nSet package name in genqlient.yaml"+ | ||
"\nExample: https://github.com/Khan/genqlient/blob/main/example/genqlient.yaml#L6", err) | ||
} | ||
// This is not likely to work if we got an error, especially if we did the | ||
// c.Package fallback. But it's more likely to work than nothing, so we may | ||
// as well. | ||
c.pkgPath = pkgPath | ||
|
||
if len(c.PackageBindings) > 0 { | ||
for _, binding := range c.PackageBindings { | ||
|
@@ -201,6 +256,11 @@ func (c *Config) ValidateAndFillDefaults(baseDir string) error { | |
binding.Package) | ||
} | ||
|
||
if binding.Package == c.pkgPath { | ||
warn(errorf(nil, "warning: package_bindings set to the same package as your generated "+ | ||
"code ('%v'); this will probably cause circularity issues", c.pkgPath)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "probably cause circular-import issues"? |
||
} | ||
|
||
mode := packages.NeedDeps | packages.NeedTypes | ||
pkgs, err := packages.Load(&packages.Config{ | ||
Mode: mode, | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
invalid config file testdata/invalid-config/InvalidCasing.yaml: unknown casing algorithm: bogus | ||
invalid config file testdata/invalidConfig/InvalidCasing.yaml: unknown casing algorithm: bogus |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
invalid config file testdata/invalid-config/InvalidOptional.yaml: optional must be one of: 'value' (default), 'pointer', or 'generic' | ||
invalid config file testdata/invalidConfig/InvalidOptional.yaml: optional must be one of: 'value' (default), 'pointer', or 'generic' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
invalid config file testdata/invalid-config/InvalidPackage.yaml: invalid package in genqlient.yaml: 'bogus-package-name' is not a valid identifier | ||
invalid config file testdata/invalidConfig/InvalidPackage.yaml: invalid package in genqlient.yaml: 'bogus-package-name' is not a valid identifier |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,5 +5,3 @@ schema: | |
operations: | ||
- first_operations.graphql | ||
- second_operations.graphql | ||
|
||
package: validConfig |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,2 @@ | ||
schema: schema.graphql | ||
operations: operations.graphql | ||
package: validConfig |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this could profitably be later down in the function, maybe lines 166- could be something like:
But I don't really understand if/how that
else
case could fire. Maybe add some documentation here.