-
Notifications
You must be signed in to change notification settings - Fork 208
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: make the api path naming plural #1013
Conversation
batch.GET("/batch/feed", s.FetchBatchFeedHandler) | ||
batch.GET("/batch/:batch_header_hash", s.FetchBatchHandler) | ||
batch.GET("/batches/feed", s.FetchBatchFeedHandler) | ||
batch.GET("/batches/:batch_header_hash", s.FetchBatchHandler) | ||
} | ||
operators := v2.Group("/operators") |
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.
Or we make it /operator/...
if it's preferred
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.
Think plural is better since the response is for multiple operators right?
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.
It can be for a single operator as well. I think it's like "this is API for dealing with multi operators" rather than saying it'll just be returning one or multiple
@@ -232,9 +232,9 @@ func TestFetchBatchHandlerV2(t *testing.T) { | |||
err = blobMetadataStore.PutAttestation(context.Background(), attestation) | |||
require.NoError(t, err) | |||
|
|||
r.GET("/v2/batch/:batch_header_hash", testDataApiServerV2.FetchBatchHandler) | |||
r.GET("/v2/batches/:batch_header_hash", testDataApiServerV2.FetchBatchHandler) |
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.
nit: Rename functions to match? FetchBatchHandler
-> FetchBatchesHandler
?
@@ -105,13 +105,13 @@ func (s *ServerV2) Start() error { | |||
{ | |||
blob := v2.Group("/blob") | |||
{ | |||
blob.GET("/blob/feed", s.FetchBlobFeedHandler) | |||
blob.GET("/blob/:blob_key", s.FetchBlobHandler) | |||
blob.GET("/blobs/feed", s.FetchBlobFeedHandler) |
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.
nit: Same comment: FetchBlob
-> FetchBlobs
?
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.
FetchBlob is fetching exactly one blob. The edge case is "FetchBlobsFeed" or "FetchBlobFeed", which I am not sure which one, maybe "blob" is adjective here so "BlobFeed" sounds right?
batch.GET("/batch/feed", s.FetchBatchFeedHandler) | ||
batch.GET("/batch/:batch_header_hash", s.FetchBatchHandler) | ||
batch.GET("/batches/feed", s.FetchBatchFeedHandler) | ||
batch.GET("/batches/:batch_header_hash", s.FetchBatchHandler) | ||
} | ||
operators := v2.Group("/operators") |
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.
Think plural is better since the response is for multiple operators right?
Why are these changes needed?
To be consistent with
/operators/...
(and plural seems more usual in such case)Checks