schemagen cli #201

Bobochka · 2021-11-13T11:58:05Z

No description provided.

mmatczuk · 2021-11-15T09:51:54Z

README.md

+  -cluster string
+    	a comma-separated list of host:port tuples (default "127.0.0.1")
+  -keyspace string
+    	keyspace to inspect (default "system")


Defaulting to system makes little sense, if not set exit with error.

mmatczuk · 2021-11-15T10:43:23Z

README.md

@@ -103,6 +103,34 @@ t.Log(people)
 // stdout: [{Michał Matczuk [michal@scylladb.com]}]
 ```

+### Generating table metadata structs with schemagen


Suggested change

### Generating table metadata structs with schemagen

## Generating table metadata with schemagen

mmatczuk · 2021-11-15T10:45:20Z

README.md

+go get -u "github.com/scylladb/gocqlx/v2/cmd/schemagen"
+```
+
+Examples:


Let's use a real example not help.

The following example generates ...

mmatczuk · 2021-11-15T10:49:58Z

cmd/schemagen/schemagen.go

+
+var (
+	//go:embed keyspace.tmpl
+	rootFs embed.FS


You can use template string instead.

Can you elaborate please? Not sure I understood.

Sure, the go embed accepts 3 things

embed.FS - if you have many files

string - if you have a single file and want it as a string

[]byte - if you have a single file

mmatczuk · 2021-11-15T10:57:59Z

cmd/schemagen/schemagen.go

+				createSession())))
+}
+
+func assert(err error, msg string) {


Panic here is not great.
I'd encourage you to use log package instead of prints and use log.Fatalf here.
This function is not very idiomatic and I'd like to see it inlined with log.Fatalf.

mmatczuk · 2021-11-15T10:59:08Z

cmd/schemagen/schemagen.go

+}
+
+func schemagen() {
+	writeToFile(


Not very idiomatic, IMO reads better squashed to a single line.

After reading more code open and handle file here (inline writeToFile) and add file as io.Writer to renderTemplate.

mmatczuk · 2021-11-15T11:00:46Z

cmd/schemagen/schemagen.go

+
+func writeToFile(b []byte) {
+	outputPath := path.Join(*flagOutput, *flagPkgname+".go")
+	ensureDir(outputPath)


Just call MkdirAll here.

mmatczuk · 2021-11-15T11:02:28Z

cmd/schemagen/schemagen.go

+	return md
+}
+
+func renderTemplate(md *gocql.KeyspaceMetadata) []byte {


It would be much better to accept io.Writer here.

mmatczuk · 2021-11-15T11:08:36Z

cmd/schemagen/schemagen.go

+	return strings.Split(*flagCluster, ",")
+}
+
+func Camelize(s string) string {


We probably do not want to export it and maybe we want to use a library instead.

mmatczuk · 2021-11-15T11:09:57Z

cmd/schemagen/schemagen_test.go

+	}
+}
+
+func resultWant(pkgname string) string {


Add a golden file in testdata dir.

mmatczuk

Please find comments inline.

Please note that assertions are not error handling.
You assert that something always holds true, asserting that user input is correct at all times is a bad idea.
Please redo assertions as idiomatic error handling.

Bobochka · 2021-11-16T07:04:03Z

@mmatczuk thanks for the comments, addressed most of them.

Asked inline regarding embed
Camelize - made unexported, should it be moved to reflectx package maybe?

cmd/schemagen/schemagen_test.go

mmatczuk · 2021-11-16T09:11:54Z

README.md

+
+Example:
+
+Running the following command for `examples` [schema](https://github.com/Bobochka/gocqlx/blob/412899a9b2910d00f9f723ea304c3abef2263021/example_test.go#L77-L97): 


The link points to your repo, I'm not sure we needed this perm link here, they tend to get obsolete.

mmatczuk · 2021-11-16T09:12:37Z

.gitignore

+cmd/schemagen/schemagen
+cmd/schemagen/models
+cmd/schemagen/asdf


Do we need that, can we generate to temp dir.

Will just remove it, tests do the clean up

mmatczuk · 2021-11-16T09:13:33Z

cmd/schemagen/schemagen.go

+	"embed"
+	"flag"
+	"fmt"
+	"github.com/scylladb/gocqlx/v2"


Please group imports.

mmatczuk · 2021-11-16T09:15:27Z

cmd/schemagen/schemagen.go

+	err = renderTemplate(f, metadata)
+	if err != nil {


Suggested change

err = renderTemplate(f, metadata)

if err != nil {

if err := renderTemplate(f, metadata); err != nil {

mmatczuk · 2021-11-16T09:16:24Z

cmd/schemagen/schemagen.go

+		log.Fatalln("unable to parse models template:", err)
+	}
+
+	buf := &bytes.Buffer{}


I think the idea here is that you do not need the buffer.

It is for the format.Source below, otherwise will need to re-read the file

mmatczuk · 2021-11-16T09:20:59Z

Overall it looks much better now.
I added some follow up inline comments.
Moving Camelize to reflectx makes indeed more sense.
Finish that first and move it with a separate PR.

mmatczuk · 2021-11-16T09:36:18Z

Can we merge?

Bobochka · 2021-11-16T11:18:31Z

Please @mmatczuk

mmatczuk · 2021-11-16T12:24:37Z

Please fix lint errors and squash commits.

Bobochka · 2021-11-16T17:09:57Z

Done

mmatczuk · 2021-11-17T08:35:36Z

Thanks!

Bobochka mentioned this pull request Nov 13, 2021

Add a code generator that would extract tables and columns from a database #4

Closed

mmatczuk reviewed Nov 15, 2021

View reviewed changes

cmd/schemagen/schemagen_test.go Outdated

}

}

func resultWant(pkgname string) string {

Copy link

Contributor

mmatczuk Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a golden file in testdata dir.

mmatczuk suggested changes Nov 15, 2021

View reviewed changes

mmatczuk reviewed Nov 16, 2021

View reviewed changes

cmd/schemagen/schemagen_test.go Outdated Show resolved Hide resolved

mmatczuk reviewed Nov 16, 2021

View reviewed changes

cmd/schemagen/schemagen.go Outdated

"embed"

"flag"

"fmt"

"github.com/scylladb/gocqlx/v2"

Copy link

Contributor

mmatczuk Nov 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please group imports.

mmatczuk reviewed Nov 16, 2021

View reviewed changes

mmatczuk approved these changes Nov 16, 2021

View reviewed changes

schemagen cli

e93c99d

mmatczuk approved these changes Nov 17, 2021

View reviewed changes

mmatczuk merged commit 1bfe101 into scylladb:master Nov 17, 2021

mmatczuk mentioned this pull request Nov 19, 2021

schemagen: generate go structs for tables #205

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemagen cli #201

schemagen cli #201

Bobochka commented Nov 13, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

Bobochka Nov 16, 2021

mmatczuk Nov 16, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk Nov 15, 2021

mmatczuk left a comment

Bobochka commented Nov 16, 2021

mmatczuk Nov 16, 2021

mmatczuk Nov 16, 2021

Bobochka Nov 16, 2021

mmatczuk Nov 16, 2021

mmatczuk Nov 16, 2021

mmatczuk Nov 16, 2021

Bobochka Nov 16, 2021

mmatczuk commented Nov 16, 2021

mmatczuk commented Nov 16, 2021

Bobochka commented Nov 16, 2021

mmatczuk commented Nov 16, 2021

Bobochka commented Nov 16, 2021

mmatczuk commented Nov 17, 2021

	### Generating table metadata structs with schemagen
	## Generating table metadata with schemagen


		Example:

		Running the following command for `examples` [schema](https://github.com/Bobochka/gocqlx/blob/412899a9b2910d00f9f723ea304c3abef2263021/example_test.go#L77-L97):

	err = renderTemplate(f, metadata)
	if err != nil {
	if err := renderTemplate(f, metadata); err != nil {

schemagen cli #201

schemagen cli #201

Conversation

Bobochka commented Nov 13, 2021

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

Choose a reason for hiding this comment

mmatczuk left a comment

Choose a reason for hiding this comment

Bobochka commented Nov 16, 2021

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

mmatczuk commented Nov 16, 2021

mmatczuk commented Nov 16, 2021

Bobochka commented Nov 16, 2021

mmatczuk commented Nov 16, 2021

Bobochka commented Nov 16, 2021

mmatczuk commented Nov 17, 2021