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

✨ feature: add AllParams method #1853

Merged
merged 1 commit into from Apr 14, 2022
Merged

Conversation

efectn
Copy link
Member

@efectn efectn commented Apr 9, 2022

Signature:
func (c *Ctx) AllParams() map[string]string

Benchmark Result:
Benchmark_Ctx_AllParams-4 2544381 530.9 ns/op 336 B/op 2 allocs/op

Closes:
#1816

@efectn efectn linked an issue Apr 9, 2022 that may be closed by this pull request
@darkweak
Copy link

darkweak commented Apr 10, 2022

I don't know if duplicate keys are supported (e.g. http://my.app/path?mykey[]=first_value&mykey[]=second_value). Shouldn't it be a map[string][]string instead of a simple map[string]string?
And you define the map length from the c.route.Params length. I guess it doesn't handle the duplicate keys right?

@ReneWerner87
Copy link
Member

I don't know if duplicate keys are supported (e.g. http://my.app/path?mykey[]=first_value&mykey[]=second_value). Shouldn't it be a map[string][]string instead of a simple map[string]string?
And you define the map length from the c.route.Params length. I guess it doesn't handle the duplicate keys right?

In your handling you describe query parameters and no resource(route) parameters

With the resource parameters or route parameters, we know beforehand how many and which ones they are, because they are predefined.

@darkweak
Copy link

Sorry for the confusion 😔

@renanbastos93
Copy link
Member

Hi dude, sorry but I don't like this approach because we have a method c.Route().Params for this.

@efectn
Copy link
Member Author

efectn commented Apr 12, 2022

Hi dude, sorry but I don't like this approach because we have a method c.Route().Params for this.

I created PR because of "Accepting PR" label 🤔. What do you think @hi019 @ReneWerner87

@ReneWerner87
Copy link
Member

This access by route is not really intuitive, I am more in favor of access by method

@renanbastos93
Copy link
Member

Hi dude, sorry but I don't like this approach because we have a method c.Route().Params for this.

I created PR because of "Accepting PR" label 🤔. What do you think @hi019 @ReneWerner87

I am so sorry, but I commented on my opinion.

Copy link
Member

@renanbastos93 renanbastos93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +838 to +839
func (c *Ctx) AllParams() map[string]string {
params := make(map[string]string, len(c.route.Params))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (c *Ctx) AllParams() map[string]string {
params := make(map[string]string, len(c.route.Params))
func (c *Ctx) AllParams() map[string][]string {
params := make(map[string][]string, len(c.route.Params))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no list, always only one parameter value for one parameter key

func (c *Ctx) AllParams() map[string]string {
params := make(map[string]string, len(c.route.Params))
for _, param := range c.route.Params {
params[param] = c.Params(param)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please adjust it based on suggestion when having many params

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look at tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't understand why to need make it []string instead of string. Can you explain me?

@ReneWerner87
Copy link
Member

Hi dude, sorry but I don't like this approach because we have a method c.Route().Params for this.

I created PR because of "Accepting PR" label 🤔. What do you think @hi019 @ReneWerner87

I am so sorry, but I commented on my opinion.

All good, everyone can freely express his opinion here

Your opinions are all welcome, helps us think about it again and reassess the whole thing

@ReneWerner87 ReneWerner87 merged commit a63a842 into gofiber:master Apr 14, 2022
trim21 pushed a commit to trim21/fiber that referenced this pull request Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How can I get all param variables?
4 participants