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 grammar and inconsistent receiver names #85

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

codefromthecrypt
Copy link
Contributor

Thank goland not me ;)

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@philippgille
Copy link
Owner

Sorry I overlooked this PR!

I'm only on mobile right now, I'll have a look on the next days.

Copy link
Owner

@philippgille philippgille left a comment

Choose a reason for hiding this comment

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

I thought I'm someone who uses too many commas 😅 and wondered about some of the changes, but looking into some of them it seems all changes lead to a generally preferred style. 👍

Thanks!

@@ -56,7 +56,7 @@ func main() {
}
// Add docs to the collection, if the collection was just created (and not
// loaded from persistent storage).
docs := []chromem.Document{}
var docs []chromem.Document
Copy link
Owner

Choose a reason for hiding this comment

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

For this one, I like to differentiate between cases where it's certain that the slice is going to be filled with elements (e.g. this code example should have 200 elements, otherwise the example is broken), vs cases where it's not certain and where you might later want to check for docs == nil (e.g. in production code with user input).

But that might just be me, and not a big deal, so no blocker

@philippgille philippgille merged commit e147c74 into philippgille:main Jun 13, 2024
4 checks passed
@codefromthecrypt codefromthecrypt deleted the fuzz branch June 14, 2024 02:16
@codefromthecrypt
Copy link
Contributor Author

thanks for pulling this in warts and all. agree with your comments!

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

2 participants