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

ReneWerner87 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?

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

darkweak commented Apr 10, 2022

Sorry for the confusion 😔

@renanbastos93
Copy link
Member

renanbastos93 commented Apr 11, 2022

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

ReneWerner87 commented Apr 12, 2022

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

@renanbastos93
Copy link
Member

renanbastos93 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

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

Copy link
Member

@renanbastos93 renanbastos93 left a comment

LGTM

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

@renanbastos93 renanbastos93 Apr 12, 2022

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

@ReneWerner87 ReneWerner87 Apr 14, 2022

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

@renanbastos93 renanbastos93 Apr 12, 2022

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

@efectn efectn Apr 12, 2022

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

@efectn efectn Apr 13, 2022

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

ReneWerner87 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

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
16 checks passed
trim21 pushed a commit to trim21/fiber that referenced this issue 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.

4 participants