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

Improved Go version of Part 2 #106

Merged
merged 6 commits into from
Jan 30, 2020
Merged

Improved Go version of Part 2 #106

merged 6 commits into from
Jan 30, 2020

Conversation

dahankzter
Copy link
Contributor

This uses github.com/scylladb/gocqlx to create a much nicer developer experience
with clear and safe code.

This is a complete transliteration of the Java code but more or
less idiomatic Go and driver config to enable shard awareness.
Slightly modified and with an asciiart twist.
This version uses github.com/scylladb/gocqlx to make the code much
more clean and readable.
@tzach
Copy link
Collaborator

tzach commented Dec 12, 2019

@dahankzter who can help with a review?

@dahankzter
Copy link
Contributor Author

dahankzter commented Dec 12, 2019 via email

@tzach tzach requested a review from mmatczuk December 12, 2019 13:18

import (
"goapp/internal/log"
"goapp/internal/scylla"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Internal packages for the module.

"github.com/scylladb/gocqlx"
"github.com/scylladb/gocqlx/qb"
"github.com/scylladb/gocqlx/table"
"go.uber.org/zap"
Copy link
Contributor

Choose a reason for hiding this comment

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

Another logger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm... Let me check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Zap is the logger. The other import just defines a helper method to create the otherwise quite verbose setup of the zap logger.

Perhaps we should use golog?

"go.uber.org/zap"
)

var (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to delete this group and use :=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are contained in a separate little helper struct now.

PictureLocation string `db:"picture_location"`
}

func init() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Init is a bad practice. Use struct instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is it bad for main apps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed init for specific global assignment.

}
}

func insertQuery(session *gocql.Session, firstName, lastName, address, pictureLocation string, logger *zap.Logger) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing all those params and building the Record is strange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I kept it for similarities with the other examples.

@guy9
Copy link
Collaborator

guy9 commented Jan 30, 2020

@tzach please merge

@tzach tzach merged commit f27d331 into scylladb:master Jan 30, 2020
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

4 participants