fix(cli): include CUEPath in BindingConfig #131

mildwonkey · 2023-04-28T14:36:56Z

The cue path (-p flag) was not being passed down to GenerateLineageBinding, so the lineage lookup was missing from generated bindings. This fixes that, adds the cue import (if cuepath is set) and finally wraps the lineage value in quotes (if not already present). Both lineage gen gobindings -p lineage and `lineage gen gobindings -p "lineage" produce the expected outcome.

for #28

Kernel: return error for target schema in validation

This is nearly there as a matter of implementation. But it needs to be refactored for smooth entry depending on whether we have a scalar, list, or struct type, proper docs need to be written, and it needs to be moved into the root thema package.

thema: Introduce `AssignableTo` func and use it to fix `NewInputKernel()` type checking

Cause `UnarySchema.successor` returns a `*UnarySchema`, returning `nil` actually becomes `*UnarySchema(nil)`. In effect, the interface returned from `UnarySchena.Successor` itself is never nil (only the boxed value is), so comparing against nil does not work

UnarySchema: use untyped nil

Fixes #38

Remove underscores in Go names

Update cuetsy to v0.1.6

Fix missing underscores

Fix previous refactor to reuse iterate functions

Fix surface Less function and add tests

… only pkg/codegen and pkg/util, which thema's go codegen utilizes, and made changes to pkg/codegen based on the unmerged deepmap/oapi-codegen PR (oapi-codegen/oapi-codegen#717) to fix bug where thema go codegen creates empty interfaces for lineages with joinSchema or any joined (&) CUE values.

…'d codegen package to the main one. Added a txtar test for joinSchema.

…egen [bugfix] Fork deepmap/oapi-codegen to fix joinSchema empty interface codegen bug

Update Cuetsy and Cuelang to the latest version

Fix oapi-codegen check

The cue path (-p flag) was not being passed down to GenerateLineageBinding, so the lineage lookup was missing from generated bindings. This fixes that and also wraps the lineage in quotes (if not already present).

sdboyer

Good catch, thank you! Just some small things

sdboyer · 2023-04-28T15:19:19Z

encoding/gocode/binding.tmpl

-    "cuelang.org/go/cue/build"
+	{{ if .CUEPath }}"cuelang.org/go/cue"{{ end }}
+	"cuelang.org/go/cue/build"


shouldn't be necessary? we have a call to goimports in the generator that eliminates unused imports if they're not needed

not a big deal, though

Well, it wasn't necessary, but this is the message thema emits without that line:

"cuelang.org/go/cue" Relying on goimports to find imports significantly slows down code generation. Either add these imports with an AST manipulation in cfg.ApplyFuncs, or set cfg.IgnoreDiscoveredImports to true```

OH!

derp, i misread the change. yes, it's definitely key to include any imports that may be present, it's just not necessary to wrap them in a conditional in the template. So...

encoding/gocode/gen.go

sdboyer · 2023-04-28T15:39:17Z

encoding/gocode/binding.tmpl

@@ -5,7 +5,8 @@ import (
 	"io/fs"
 	"path/filepath"

-    "cuelang.org/go/cue/build"
+	{{ if .CUEPath }}"cuelang.org/go/cue"{{ end }}


...this is what i should have suggested 😄

Suggested change

{{ if .CUEPath }}"cuelang.org/go/cue"{{ end }}

"cuelang.org/go/cue"

That's not better - if we aren't setting the cue path, we're not adding a line with cue.Path, so that import will be removed.

right, this is more of a consistency nit than anything. There are other imports in that list - at least embed and io/fs - which are only conditionally necessary, as well. But they're not wrapped in a conditional.

The conclusion i came to after writing a few of these templates is that it's easier to maintain a template with the full set of imports that may be necessary and rely on goimports to remove unnecessary ones for particular cases, than to have the additional conditionals in the template file.

Ok, I can do that. Can I remove the warning about relying on goimports, if that's now how expected?

IIUC there shouldn't be a warning about goimports now, because that warning only happens if goimports is trying to add an import, not if it removes one.

And yet, when I ran the command, I got a warning, which is why I made the change. I'd like to either add the import or remove the warning.

sdboyer · 2023-04-28T16:15:59Z

cmd/thema/thema

oop, also need to remove the committed binary

i'd ask that this be rebased out so that it's not in the git commit history 🙏🏻

i just pushed a commit to main to add this to gitignore

sam boyer and others added 30 commits January 23, 2022 23:09

cmd/thema: fix command use

5fd586d

Kernel: (wip) return error for target schema in validation

d21eb4b

for #28

still validateany

b0d1a91

Merge pull request #34 from grafana/inputKernelValidationErrors

37f7db1

Kernel: return error for target schema in validation

Split walker into three funcs for flexibility

d535ec0

Little comments on the table test

2d96cf8

Write up assignability rules in invariants doc

399b676

Fix up assignability docs a bit more

5cedbaa

Move assignable func impl into thema package

f41ed42

Move TestAssignable into thema package

a3cf29c

Remove pointer type requirement from assignable

f4c7c49

Tighten optional relation requirements

6d628fa

Handle list element type variance

5efbf78

Use new AssignableTo() func in NewInputKernel()

05ac3b9

Clarify number-type guidance, clean up code

1bf6da2

Merge pull request #37 from grafana/improve-assignable-check

4e10a89

thema: Introduce `AssignableTo` func and use it to fix `NewInputKernel()` type checking

thema: Massage README text some more

5d5c169

docs: List indent linting

eb42ab4

readme: Add prior art section

10153a2

Merge pull request #41 from grafana/typed-nil

f90c5c5

UnarySchema: use untyped nil

Add srv and gen commands

208ee50

Add nolint marker

0f75f7d

kernel: Use bare interface{} with InputKernel

8ead843

Fixes #38

lint: Add Close() calls, include linting

4d8a90f

thema: Add escape hatch for interface{}/any

c434b34

thema: Handle defaults in lists properly

57d4af0

thema: Add check for default struct assignability

061b606

errors: Add some new validation error categories

1ed4cd7

AgnesToulet and others added 23 commits February 21, 2023 17:54

Merge branch 'main' into remove-go-underscores

1754b55

Merge pull request #113 from grafana/remove-go-underscores

014ccb5

Remove underscores in Go names

Update cuetsy to v0.1.6

9851847

Merge pull request #114 from grafana/update-cuetsy-0.1.6

5f40f38

Update cuetsy to v0.1.6

It misses struct underscores

f20da21

Typo

90e75ef

Itereate maps and update tests

c38cd47

Reuse iterate functions

ccfcd08

Merge pull request #115 from grafana/fix-missing-underscores

870dd3d

Fix missing underscores

Fix previous refactor to reuse iterate functions

f3aa189

Merge pull request #116 from grafana/fix-decorator-refactor

ed581f2

Fix previous refactor to reuse iterate functions

Fix surface Less function and add tests

7ae2792

fix one test case

0df3eeb

Merge pull request #117 from grafana/fix-less

9f71d0d

Fix surface Less function and add tests

Add CONTRIBUTING.md to internal/deepmap as well as a diff of the fork…

0481450

…'d codegen package to the main one. Added a txtar test for joinSchema.

Merge pull request #127 from grafana/bugfix/124-fix-joinschema-go-cod…

39b66bc

…egen [bugfix] Fork deepmap/oapi-codegen to fix joinSchema empty interface codegen bug

Update Cuetsy to the last version

c500395

Merge branch 'main' of github.com:grafana/thema into cuetsy-v0.1.8

ad1fbde

Merge pull request #128 from grafana/cuetsy-v0.1.8

e8d99c3

Update Cuetsy and Cuelang to the latest version

Fix buggy check

8799b84

Merge pull request #129 from grafana/fix-buggy-check

7dd44af

Fix oapi-codegen check

fix(cli): include CUEPath provided on CLI to BindingConfig

9e9efc4

The cue path (-p flag) was not being passed down to GenerateLineageBinding, so the lineage lookup was missing from generated bindings. This fixes that and also wraps the lineage in quotes (if not already present).

mildwonkey requested a review from sdboyer April 28, 2023 15:13

sdboyer suggested changes Apr 28, 2023

View reviewed changes

sdboyer reviewed Apr 28, 2023

View reviewed changes

mildwonkey closed this May 1, 2023

mildwonkey force-pushed the mildwonkey/lin-path branch from 3661c17 to 9e9efc4 Compare May 1, 2023 12:42

sdboyer mentioned this pull request Jul 14, 2023

gocode: Fix subpath generation in bindings #184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): include CUEPath in BindingConfig #131

fix(cli): include CUEPath in BindingConfig #131

mildwonkey commented Apr 28, 2023

sdboyer left a comment

sdboyer Apr 28, 2023

mildwonkey Apr 28, 2023

sdboyer Apr 28, 2023

sdboyer Apr 28, 2023

mildwonkey Apr 28, 2023

sdboyer Apr 28, 2023

mildwonkey Apr 28, 2023

sdboyer Apr 28, 2023

mildwonkey May 1, 2023

sdboyer Apr 28, 2023

sdboyer Apr 28, 2023

	{{ if .CUEPath }}"cuelang.org/go/cue"{{ end }}
	"cuelang.org/go/cue"

fix(cli): include CUEPath in BindingConfig #131

fix(cli): include CUEPath in BindingConfig #131

Conversation

mildwonkey commented Apr 28, 2023

sdboyer 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

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