-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
✨ (v3) feature: Implement new generic functions: Params, Get and Convert #2850
Conversation
What's the benchmark difference between using this and the previous implementation? |
I'll implement this. |
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.
otherwise LGTM
Co-authored-by: Jason McNeil <sixcolors@mac.com>
Wait for #2842 to get merged. I suspect a lot of warnings will pop up |
Ok. I'll wait. |
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.
thx, some adjustments needed
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ctx_test.go (6 hunks)
Files not summarized due to errors (1)
- ctx_test.go: Error: Message exceeds token limit
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2850 +/- ##
==========================================
- Coverage 82.84% 82.73% -0.12%
==========================================
Files 116 116
Lines 8377 8385 +8
==========================================
- Hits 6940 6937 -3
- Misses 1098 1108 +10
- Partials 339 340 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
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.
@gaby @efectn @nickajacks1 otherwise leave an approval so i can merge in the next days |
I'll try to do a final look over this weekend |
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.
Nothing in particular stands out
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ctx_test.go (6 hunks)
Files not summarized due to errors (1)
- ctx_test.go: Error: Message exceeds token limit
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.
In certain scenarios, it can be useful to have an alternative approach to handle different types of parameters, not | ||
just strings. This can be achieved using a generic Query function known as `Params[V GenericType](c Ctx, key string, defaultValue ...V) V`. | ||
This function is capable of parsing a query string and returning a value of a type that is assumed and specified by `V GenericType`. | ||
|
||
```go title="Signature" | ||
func (c Ctx) ParamsInt(key string) (int, error) | ||
func Params[v GenericType](c Ctx, key string, default value ...V) V | ||
``` | ||
|
||
```go title="Example" | ||
// GET http://example.com/user/123 | ||
app.Get("/user/:id", func(c fiber.Ctx) error { | ||
id, err := c.ParamsInt("id") // int 123 and no error | ||
|
||
// ... | ||
// Get http://example.com/user/114 | ||
app.Get("/user/:id", func(c fiber.Ctx) error{ | ||
fiber.Params[string](c, "id") // returns "114" as string. | ||
fiber.Params[int](c, "id") // returns 114 as integer | ||
fiber.Params[string](c, "number") // retunrs "" (default string type) | ||
fiber.Params[int](c, "number") // returns 0 (default integer value type) | ||
}) | ||
|
||
``` | ||
|
||
This method is equivalent of using `atoi` with ctx.Params | ||
The generic Params function supports returning the following data types based on V GenericType: | ||
- Integer: int, int8, int16, int32, int64 | ||
- Unsigned integer: uint, uint8, uint16, uint32, uint64 | ||
- Floating-point numbers: float32, float64 | ||
- Boolean: bool | ||
- String: string | ||
- Byte array: []byte |
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.
The introduction of generic versions for the Params
and Query
methods is a significant enhancement. However, it's crucial to ensure that the documentation clearly explains how to use these generic versions, including detailed examples for various data types. Additionally, consider adding a note on potential pitfalls or common mistakes to avoid when using generics in this context.
Would you like me to help with adding examples or notes on common pitfalls?
## Convert | ||
Converts a string value to a specified type, handling errors and optional default values. | ||
This function simplifies the conversion process by encapsulating error handling and the management of default values, making your code cleaner and more consistent. | ||
```go title="Signature" | ||
func Convert[T any](value string, convertor func(string) (T, error), defaultValue ...T) (*T, error) | ||
``` | ||
|
||
```go title="Example" | ||
// GET http://example.com/id/bb70ab33-d455-4a03-8d78-d3c1dacae9ff | ||
app.Get("/id/:id", func(c fiber.Ctx) error { | ||
fiber.Convert(c.Params("id"), uuid.Parse) // UUID(bb70ab33-d455-4a03-8d78-d3c1dacae9ff), nil | ||
|
||
|
||
// GET http://example.com/search?id=65f6f54221fb90e6a6b76db7 | ||
app.Get("/search", func(c fiber.Ctx) error) { | ||
fiber.Convert(c.Query("id"), mongo.ParseObjectID) // objectid(65f6f54221fb90e6a6b76db7), nil | ||
fiber.Convert(c.Query("id"), uuid.Parse) // uuid.Nil, error(cannot parse given uuid) | ||
fiber.Convert(c.Query("id"), uuid.Parse, mongo.NewObjectID) // new object id generated and return nil as error. | ||
} | ||
|
||
// ... | ||
}) |
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.
The Convert
function is a valuable addition, simplifying the conversion of string values to specified types with error handling. It's important to ensure that the documentation for this function is clear and includes examples of its usage in different scenarios. This will help users understand how to effectively utilize the Convert
function in their applications.
Would you like assistance in drafting examples or additional documentation for the Convert
function?
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- ctx_test.go (6 hunks)
Files not summarized due to errors (1)
- ctx_test.go: Error: Message exceeds token limit
func Test_genericParseTypeUint8s(t *testing.T) { | ||
t.Parallel() | ||
type genericTypes[v GenericType] struct { | ||
value v | ||
str string | ||
} | ||
|
||
uint8s := []genericTypes[uint8]{ | ||
{ | ||
value: uint8(0), | ||
str: "0", | ||
}, | ||
{ | ||
value: uint8(1), | ||
str: "1", | ||
}, | ||
{ | ||
value: uint8(2), | ||
str: "2", | ||
}, | ||
{ | ||
value: uint8(3), | ||
str: "3", | ||
}, | ||
{ | ||
value: uint8(4), | ||
str: "4", | ||
}, | ||
{ | ||
value: uint8(math.MaxUint8), | ||
str: strconv.Itoa(math.MaxUint8), | ||
}, | ||
} | ||
|
||
for _, test := range uint8s { | ||
var v uint8 | ||
tt := test | ||
t.Run("test_genericParseTypeUint8s", func(t *testing.T) { | ||
t.Parallel() | ||
require.Equal(t, tt.value, genericParseType(tt.str, v)) | ||
require.Equal(t, tt.value, genericParseType[uint8](tt.str, v)) | ||
}) | ||
} | ||
} |
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.
The test function Test_genericParseTypeUint8s
is well-structured and tests the conversion of strings to uint8 values. It's recommended to include edge cases such as the minimum and maximum values for uint8.
uint8s = append(uint8s, genericTypes[uint8]{value: math.MaxUint8, str: strconv.Itoa(math.MaxUint8)})
uint8s = append(uint8s, genericTypes[uint8]{value: 0, str: strconv.Itoa(0)})
Description
This pull request introduces new features, Params, Get and Convert generic functions.
With the
Params
andGet
generic functions you can parse your parameters and headers in different data types.Also With the Convert function, you can parse your data types in what type you want, like
uuid.UUID
.TODO:
ctx.Params
ctx.Get
This pull request will be fixed #2758
Summary by CodeRabbit
New Features
Refactor
Documentation