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

fix: Honor Package level renames in v2 yaml config #2001

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

Emyrk
Copy link
Contributor

@Emyrk Emyrk commented Dec 15, 2022

When following the docs for renames for pkg scoped renames, the renames were not applying. This is my first look into the sqlc config, but it looked like things were just not plumbed through.

I just adds setting the renames from the yaml parse. In the combine settings I made sure to assign the renames from the pkg (the global was already set). This change does make pkg renames override global ones.


For unit testing, I noticed all the endtoend/testdata use a v1 config. Instead of adding more vectors to that to also do v2 directories, what if we just added a sqlc.v1.yaml and sqlc.v2.yaml to each directory and run it twice? Or something of that nature to test v2 configs? At present this is not tested aside from some manual checking.

Package level config renames override global level config renames
@alicebob
Copy link

This seems to fix #1977.

@Emyrk
Copy link
Contributor Author

Emyrk commented Feb 14, 2023

Is it possible to get this in soon? Seems small and trivial?

@mpyw
Copy link
Contributor

mpyw commented Feb 15, 2023

I noticed the same problem and wrote a patch, but then I found that this PR had already been submitted! 🤣

diff --git a/internal/config/config.go b/internal/config/config.go
index 0dfd57bc..1c8942c8 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -214,12 +214,22 @@ func Combine(conf Config, pkg SQL) CombinedSettings {
 		Global:  conf,
 		Package: pkg,
 	}
+	merge := func(dst, src map[string]string) map[string]string {
+		for k, v := range src {
+			if dst == nil {
+				dst = make(map[string]string)
+			}
+			dst[k] = v
+		}
+		return dst
+	}
 	if conf.Gen.Go != nil {
-		cs.Rename = conf.Gen.Go.Rename
+		cs.Rename = merge(cs.Rename, conf.Gen.Go.Rename)
 		cs.Overrides = append(cs.Overrides, conf.Gen.Go.Overrides...)
 	}
 	if pkg.Gen.Go != nil {
 		cs.Go = *pkg.Gen.Go
+		cs.Rename = merge(cs.Rename, pkg.Gen.Go.Rename)
 		cs.Overrides = append(cs.Overrides, pkg.Gen.Go.Overrides...)
 	}
 	if pkg.Gen.JSON != nil {

@mpyw
Copy link
Contributor

mpyw commented Feb 15, 2023

@kyleconroy There are a workaround with global rename config, but the bug may confuse many people. A patch version should be released soon if possible.

@sgielen
Copy link

sgielen commented Mar 21, 2023

@kyleconroy respectful bump to make you aware of this tiny PR fixing #1977 :-)

@Emyrk
Copy link
Contributor Author

Emyrk commented Mar 21, 2023

@kyleconroy There are a workaround with global rename config, but the bug may confuse many people. A patch version should be released soon if possible.

That is what I did in the meantime.

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.

None yet

5 participants