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

devel_glfw3.1: incorrect renaming of Char to Character? #105

Closed
slimsag opened this issue Dec 4, 2014 · 6 comments
Closed

devel_glfw3.1: incorrect renaming of Char to Character? #105

slimsag opened this issue Dec 4, 2014 · 6 comments
Assignees

Comments

@slimsag
Copy link
Member

slimsag commented Dec 4, 2014

It looks like we renamed these functions spelling out Char as Character.

For example glfwSetCharCallback -> SetCharacterCallback spelled out fully. We did it with SetCharacterModsCallback too.

This isn't a big deal but it seems rather inconsistent I think. What say you guys?

@tapir
Copy link
Member

tapir commented Dec 4, 2014

I don't remember the reasoning behind it but it seems inconsistent with GLFW API.
Sometimes there are name collisions between funcs and structs and in those cases I might have done this to prevent the collision but this is not the case here.

My guess is that, fully spelled version is more Go-like and readable. If there is strong opinion to follow GFLW API strictly I'm not against it.

@slimsag
Copy link
Member Author

slimsag commented Dec 4, 2014

I see. Thanks for the prompt response!

If there is strong opinion to follow GFLW API strictly I'm not against it.

My main argument is that, unless there is a good reason, it is a needless place where someone who has used the C API (or any other language bindings, I guess) would possibly trip.

@dmitshur
Copy link
Member

dmitshur commented Dec 4, 2014

I think it's better just to keep the spelling the same as GLFW C library. Unless there are very pressing (more so than just better sounding name) reasons to do otherwise.

@dmitshur dmitshur self-assigned this Jan 4, 2015
@dmitshur
Copy link
Member

dmitshur commented Jan 4, 2015

I will make a PR that changes the naming to be consistent with GLFW names. Let me know if there are any objections, but I think that's the best thing to do.

@dmitshur
Copy link
Member

dmitshur commented Jan 5, 2015

Just a thought,

someone who has used the C API (or any other language bindings, I guess) would possibly trip.

Isn't that unlikely since Go is statically typed, and there's godoc.org and likely autocompletion available? If someone tries to write window.SetCharCallback(...), they'll get an error:

./main.go:123: window.SetCharCallback undefined (type *glfw3.Window has no field or method SetCharCallback)

Then they'll go godoc.org and see that it's called something else. Or, more likely, they'll be looking at godoc.org to begin with (how else do they have a window, etc.), or their autocompletion will suggest aracterCallback when they type window.SetChar:

image

Nevertheless, I still believe sticking with original upstream API func names where possible is simpler/better. Even if resolving this inconsistency is easy, it's better if it doesn't exist in the first place. Also, porting code will be easier as there'll be less manual/irregular changes.

Making a PR to close this issue now.

@slimsag
Copy link
Member Author

slimsag commented Jan 20, 2015

Closing this as the PR has been merged into the devel_glfw3.1 branch.

@slimsag slimsag closed this as completed Jan 20, 2015
dmitshur added a commit that referenced this issue Feb 22, 2015
This adds a breaking API change by renaming `SetCharacterCallback` to and `SetCharCallback`, and `SetCharacterModsCallback` to `SetCharModsCallback`. Updating is easy thanks to static type checking, and can be automated with `gofmt`:

	gofmt -w -r 'x.SetCharacterCallback -> x.SetCharCallback' *.go
	gofmt -w -r 'x.SetCharacterModsCallback -> x.SetCharModsCallback' *.go

Closes #105.
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

No branches or pull requests

3 participants