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

Mixed pointer and value recivers #130

Closed
andreykyz opened this issue Jan 30, 2024 · 4 comments
Closed

Mixed pointer and value recivers #130

andreykyz opened this issue Jan 30, 2024 · 4 comments
Labels
wontfix This will not be worked on

Comments

@andreykyz
Copy link

https://github.com/gofrs/uuid/blob/22c52c268bc0dcc0569793f5b1433db423f5a9c6/uuid.go#L286C1-L286C36
according to https://go.dev/tour/methods/8

In general, all methods on a given type should have either value or pointer receivers, but not a mixture of both. (We'll see why over the next few pages.)

@zerkms
Copy link
Member

zerkms commented Jan 30, 2024

https://go.dev/tour is a beginners tutorial to the language. Does it cause any demonstrable issues?

@dylan-bourque
Copy link
Member

Does it cause any demonstrable issues?

I'll second this point. Much of the code in this repo came from github.com/satori/go.uuid so we (the current maintainers) aren't fully responsible for the decisions that were made there. I'd argue against making a purely cosmetic change for this.

FWIW, I'd support following this "rule" for any new code.

@andreykyz
Copy link
Author

andreykyz commented Feb 9, 2024

Does it cause any demonstrable issues?

Pointer receiver means that instance can be nil instead of value receiver which cause segfault in that situation.

@andreykyz
Copy link
Author

FWIW, I'd support following this "rule" for any new code.

There are much better UUID library.

@theckman theckman closed this as completed Feb 9, 2024
@gofrs gofrs locked as resolved and limited conversation to collaborators Feb 9, 2024
@cameracker cameracker added the wontfix This will not be worked on label May 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

5 participants