-
Notifications
You must be signed in to change notification settings - Fork 98
feat: method namer #130
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
feat: method namer #130
Changes from 3 commits
15eee17
fe5d436
57d1697
a86ba49
19da84b
e72f58d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -104,6 +104,8 @@ type client struct { | |||||||||||||
| doRequest func(context.Context, clientRequest) (clientResponse, error) | ||||||||||||||
| exiting <-chan struct{} | ||||||||||||||
| idCtr int64 | ||||||||||||||
|
|
||||||||||||||
| methodNameFormatter MethodNameFormatter | ||||||||||||||
rvagg marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewMergeClient is like NewClient, but allows to specify multiple structs | ||||||||||||||
|
|
@@ -138,9 +140,10 @@ func NewCustomClient(namespace string, outs []interface{}, doRequest func(ctx co | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| c := client{ | ||||||||||||||
| namespace: namespace, | ||||||||||||||
| paramEncoders: config.paramEncoders, | ||||||||||||||
| errors: config.errors, | ||||||||||||||
| namespace: namespace, | ||||||||||||||
| methodNameFormatter: config.methodNamer, | ||||||||||||||
| paramEncoders: config.paramEncoders, | ||||||||||||||
| errors: config.errors, | ||||||||||||||
|
||||||||||||||
| methodNameFormatter: config.methodNamer, | |
| paramEncoders: config.paramEncoders, | |
| errors: config.errors, | |
| paramEncoders: config.paramEncoders, | |
| errors: config.errors, | |
| methodNameFormatter: config.methodNamer, |
retain ordering of the fields as defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Done
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Done
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Done
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,26 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package jsonrpc | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import "strings" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // MethodNameFormatter is a function that takes a namespace and a method name and returns the full method name, sent via JSON-RPC. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This is useful if you want to customize the default behaviour, e.g. send without the namespace or make it lowercase. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type MethodNameFormatter func(namespace, method string) string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // DefaultMethodNameFormatter joins the namespace and method name with a dot. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func DefaultMethodNameFormatter(namespace, method string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return namespace + "." + method | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NoNamespaceMethodNameFormatter returns the method name as is, without the namespace. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NoNamespaceMethodNameFormatter(_, method string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return method | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // NoNamespaceDecapitalizedMethodNameFormatter returns the method name as is, without the namespace, and decapitalizes the first letter. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // e.g. "Inc" -> "inc" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func NoNamespaceDecapitalizedMethodNameFormatter(_, method string) string { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(method) == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return strings.ToLower(method[:1]) + method[1:] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // DefaultMethodNameFormatter joins the namespace and method name with a dot. | |
| func DefaultMethodNameFormatter(namespace, method string) string { | |
| return namespace + "." + method | |
| } | |
| // NoNamespaceMethodNameFormatter returns the method name as is, without the namespace. | |
| func NoNamespaceMethodNameFormatter(_, method string) string { | |
| return method | |
| } | |
| // NoNamespaceDecapitalizedMethodNameFormatter returns the method name as is, without the namespace, and decapitalizes the first letter. | |
| // e.g. "Inc" -> "inc" | |
| func NoNamespaceDecapitalizedMethodNameFormatter(_, method string) string { | |
| if len(method) == 0 { | |
| return "" | |
| } | |
| return strings.ToLower(method[:1]) + method[1:] | |
| } | |
| // CaseStyle represents the case style for method names. | |
| type CaseStyle int | |
| const ( | |
| OriginalCase CaseStyle = iota | |
| LowerFirstCharCase | |
| ) | |
| // NewMethodNameFormatter creates a new method name formatter based on the provided options. | |
| func NewMethodNameFormatter(includeNamespace bool, nameCase CaseStyle) MethodNameFormatter { | |
| return func(namespace, method string) string { | |
| formattedMethod := method | |
| if nameCase == LowerFirstCharCase && len(method) > 0 { | |
| formattedMethod = strings.ToLower(method[:1]) + method[1:] | |
| } | |
| if includeNamespace { | |
| return namespace + "." + formattedMethod | |
| } | |
| return formattedMethod | |
| } | |
| } | |
| // DefaultMethodNameFormatter is a pass-through formatter with default options. | |
| var DefaultMethodNameFormatter = NewMethodNameFormatter(true, OriginalCase) |
How about something like this to solve the weird naming problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Done
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| package jsonrpc | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "github.com/stretchr/testify/require" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestDifferentMethodNamers(t *testing.T) { | ||
| tests := map[string]struct { | ||
| namer MethodNameFormatter | ||
|
|
||
| requestedMethod string | ||
| }{ | ||
| "default namer": { | ||
| namer: DefaultMethodNameFormatter, | ||
| requestedMethod: "SimpleServerHandler.Inc", | ||
| }, | ||
| "no namespace namer": { | ||
| namer: NoNamespaceMethodNameFormatter, | ||
| requestedMethod: "Inc", | ||
| }, | ||
| "no namespace & decapitalized namer": { | ||
| namer: NoNamespaceDecapitalizedMethodNameFormatter, | ||
| requestedMethod: "inc", | ||
| }, | ||
| } | ||
| for name, test := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| rpcServer := NewServer(WithServerMethodNameFormatter(test.namer)) | ||
|
|
||
| serverHandler := &SimpleServerHandler{} | ||
| rpcServer.Register("SimpleServerHandler", serverHandler) | ||
|
|
||
| testServ := httptest.NewServer(rpcServer) | ||
| defer testServ.Close() | ||
|
|
||
| req := fmt.Sprintf(`{"jsonrpc": "2.0", "method": "%s", "params": [], "id": 1}`, test.requestedMethod) | ||
|
|
||
| res, err := http.Post(testServ.URL, "application/json", strings.NewReader(req)) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, http.StatusOK, res.StatusCode) | ||
| require.Equal(t, int32(1), serverHandler.n) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestDifferentMethodNamersWithClient(t *testing.T) { | ||
| tests := map[string]struct { | ||
| namer MethodNameFormatter | ||
| urlPrefix string | ||
| }{ | ||
| "default namer & http": { | ||
| namer: DefaultMethodNameFormatter, | ||
| urlPrefix: "http://", | ||
| }, | ||
| "default namer & ws": { | ||
| namer: DefaultMethodNameFormatter, | ||
| urlPrefix: "ws://", | ||
| }, | ||
| "no namespace namer & http": { | ||
| namer: NoNamespaceMethodNameFormatter, | ||
| urlPrefix: "http://", | ||
| }, | ||
| "no namespace namer & ws": { | ||
| namer: NoNamespaceMethodNameFormatter, | ||
| urlPrefix: "ws://", | ||
| }, | ||
| "no namespace & decapitalized namer & http": { | ||
| namer: NoNamespaceDecapitalizedMethodNameFormatter, | ||
| urlPrefix: "http://", | ||
| }, | ||
| "no namespace & decapitalized namer & ws": { | ||
| namer: NoNamespaceDecapitalizedMethodNameFormatter, | ||
| urlPrefix: "ws://", | ||
| }, | ||
| } | ||
| for name, test := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| rpcServer := NewServer(WithServerMethodNameFormatter(test.namer)) | ||
|
|
||
| serverHandler := &SimpleServerHandler{} | ||
| rpcServer.Register("SimpleServerHandler", serverHandler) | ||
|
|
||
| testServ := httptest.NewServer(rpcServer) | ||
| defer testServ.Close() | ||
|
|
||
| var client struct { | ||
| AddGet func(int) int | ||
| } | ||
|
|
||
| closer, err := NewMergeClient( | ||
| context.Background(), | ||
| test.urlPrefix+testServ.Listener.Addr().String(), | ||
| "SimpleServerHandler", | ||
| []any{&client}, | ||
| nil, | ||
| WithHTTPClient(testServ.Client()), | ||
| WithMethodNameFormatter(test.namer), | ||
| ) | ||
| require.NoError(t, err) | ||
| defer closer() | ||
|
|
||
| n := client.AddGet(123) | ||
| require.Equal(t, 123, n) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this name, but
NoNamespaceLowerCamelCaseMethodNameFormatterwould be getting a bit out of hand I thinkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ I've adjusted it according to your idea with
NewMethodNameFormatter