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

Include error constants in includes in this repo? #32

Open
paddybyers opened this issue Aug 31, 2018 · 6 comments
Open

Include error constants in includes in this repo? #32

paddybyers opened this issue Aug 31, 2018 · 6 comments

Comments

@paddybyers
Copy link
Member

Instead of expecting the libraries all to use explicit numeric constants for error codes, we can generate constants for them, and have include files in this repo; for example for go:

package ably

// ably error codes
const (
	ErrNothing                                                       = 10000
	ErrBadRequest                                                    = 40000
	ErrInvalidRequestBody                                            = 40001
	ErrInvalidParameterName                                          = 40002
	ErrInvalidParameterValue                                         = 40003
	ErrInvalidHeader                                                 = 40004
	ErrInvalidCredential                                             = 40005
	ErrInvalidConnectionID                                           = 40006
	ErrInvalidMessageID                                              = 40007
	ErrInvalidContentLength                                          = 40008
	ErrMaximumMessageLengthExceeded                                  = 40009
	ErrInvalidChannelName                                            = 40010
	...
	ErrMemberImplicitlyLeftPresenceChannelConnectionClosed           = 91100
)

This was generated by a script, and could easily also be done for any other language.

I propose we add this file at <repo>/protocol/include/errors.go, add the script somewhere, and add the other includes as and when we're updating the relevant libraries.

@mattheworiordan @SimonWoolf wdyt?

cc @gernest

@paddybyers
Copy link
Member Author

It has been pointed out that this would require the submodule to be present for users just getting and building the library (ie not just library developers), so perhaps too much of an inconvenience.

Perhaps just add the script here, not the include file, so other libs can generate a consistent set of constants in their own language.

@mattheworiordan
Copy link
Member

See ably/ably-ruby#171. I think it's nice, and my approach does not require access to the errors JSON file at runtime. However, in untyped languages there is a risk things could break badly if the wording of an error code is changed.

@SimonWoolf
Copy link
Member

SimonWoolf commented Sep 3, 2018

there is a risk things could break badly if the wording of an error code is changed.

Might be better have each entry in errors.json be an object with separate name and description fields (with the name being in some canonical casing that client libs can parse & tweak to their desire), rather than try and autogenerate names from the description (meaning we can never update/clarify a human-readable description once it's been added)? Bonus: the error log could source the latest description automatically.

Definitely agree that we shouldn't require access to the submodule at runtime. @mattheworiordan's approach of having a task in the client lib repo to generate (and commit) a language-specific errorcode file using the submodule (and so make sure the result passes ci before releasing it) seems like the right approach 👍

@gernest
Copy link

gernest commented Sep 3, 2018

@paddybyers , I think @SimonWoolf has a solid argument. Even if we have this tool, still more work will need to be done in tooling and also templates for language specific things.

One example is, constants in go AreCamelCase while other languages are UPPER_CASE this is trivial, however there are nitpicks involved for instance .

from @mattheworiordan INVALID_USE_OF_BASIC_AUTH_OVER_NONTLS_TRANSPORT is idiomatic ruby , but for go the same constant will need to be InvalidUseOfBasicAuthOverNonTLSTransport see how NONTLS ==> NonTLS . I believe there should also be some landmines in other languages too. You can pull this off with go templates but it will require a shitload of go templates wizardly.

@ORBAT what is your input on this? I think I will halt the PR work warranting further discussion.

@ORBAT
Copy link

ORBAT commented Sep 3, 2018

I agree that there should be no "runtime" dependency on the script (see this ably-ruby issue), but I think this would be a valuable tool.

Whether you should do it now is another question. I'd say yes, since it should be fairly fast to do and it's needed anyhow.

As for the casing, you could use a package like https://github.com/iancoleman/strcase to provide template functions, so each template could specify their preferred case.

gernest added a commit to gernest/ably-common that referenced this issue Sep 3, 2018
@mattheworiordan
Copy link
Member

I still think JSON is far more portable than anything else we do. So whilst of course we could get a central repo to generate language specific code, it will also need to know things like namespaces / class defs / structs to create a file that is usable in the correct format. As such, the developer of their client libs has a dependency on not just data, but now actual code from a 3rd party lib they do not control.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

5 participants