Closed taniabogatsch closed 6 months ago
Another question. What is the policy around blank lines? I just saw this commit. Should this be included in a formatter run?
I typically run golangci-lint
over the PR to make sure, the low hanging fruits are catched. Maybe we can add a this as a hint to the style guide section:
Running the following Docker command in the go-duckdb
directory should be fine:
docker run --rm -v $(pwd):/app -w /app golangci/golangci-lint golangci-lint run -v
Currently there is one false positive in the old appender:
appender.go:138:2: SA4006: this value of `defaultLogicalType` is never used (staticcheck)
defaultLogicalType := C.duckdb_create_logical_type(0)
I've added the golangci-lint
run to the workflows. The false positive should resolve after merging #150 🤞 I think it might not be necessary to initialize the type there, so it might not even be a false positive.
@marcboeker, do you mind merging this, or should we wait until #167, which fixes the remaining linter issues, is merged?
@marcboeker, do you mind merging this, or should we wait until #167, which fixes the remaining linter issues, is merged?
I've locally merged main (including #167) into your branch and golangci-lint does not throw any errors. So I'm happy with merging them. Thanks for integrating it and coming up with the idea for comprehensible contribution guidelines.
https://github.com/marcboeker/go-duckdb/pull/150 saw a lot of refactoring. I wrote up an initial version of potential contributing guidelines to improve future contributions. The motivation behind this document is to increase transparency for contributors about requested changes to their PRs. The document can be iterated in the future with different sections for
PR
,CI
,Testing
,Formatting
, andGo Guidelines
.For now, I also want to use this PR to clarify questions that arose after the changes to #150. It would be nice if refactoring happens on the contributor's side before merging a PR and without requiring excessive comments from the reviewer.
Coming from C++, I know that the Go community sees a lot of discussions about
idiomatic Go
. To my knowledge, the official document on this is effective Go. I would like to know what you're expecting in this project beyond what's mentioned in that document.I tried to find direction for the following points, but please bear with me if I missed sections.
Commentary
Effective Go does not specify formatting for comments for humans. Given the following edit, I assume that for this project, the commenting style is
// Uppercase sentence. Single-space next sentence.
?Error messages: And what about error messages? From your changes, I suspect they should be
lowercase message and no dot at the end
Names
To the best of my knowledge, the effective Go documentation only specifies mixed caps.
Beyond that, function-local variable names seem to depend on the developer/repository/project? I saw the renaming of variable names such as this. Is there more orientation on variable naming beyond personal flavor? I saw this link, which suggests
cschema
as idiomatic Go.Given this change (
columnInfo -> colInfo
), I am unsure when to abbreviate a name and when to write it out.Formatting
I also saw changes like this, and other places introducing new lines. Would a CI run to ensure formatting rules make sense? Even if it only runs
go fmt
and some additional formatting rules, such as the maximum line length?Function order
Is there a guideline on function ordering in a file? I.e., I saw that you moved functions as part of the reordering. I found this link.