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

fix/validate-args-length #2823 #3974

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ArjitGopher
Copy link

@ArjitGopher ArjitGopher commented Oct 2, 2024

What?

This PR aims to solve the issue #2823

Why?

  1. The changes made will warn the user if more than two arguments are passed in http.get and http.head method
  2. A helper function is added.

Signed-off-by: arjitGopher <agarwalarjit.agarwal@gmail.com>
@ArjitGopher ArjitGopher requested a review from a team as a code owner October 2, 2024 17:26
@ArjitGopher ArjitGopher requested review from olegbespalov and joanlopez and removed request for a team October 2, 2024 17:26
@CLAassistant
Copy link

CLAassistant commented Oct 2, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines 64 to 68
validateArgCount := func(methodName string,args ...sobek.Value){
if len(args)>2 {
vu.State().Logger.Warningf("%s method has more than two arguments",methodName)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
validateArgCount := func(methodName string,args ...sobek.Value){
if len(args)>2 {
vu.State().Logger.Warningf("%s method has more than two arguments",methodName)
}
}
validateArgCount := func(methodName string,args ...sobek.Value){
if len(args)>2 {
vu.State().Logger.Warnf("http.%s method has more than two arguments",methodName)
}
}

Small suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree with @joanlopez that it's better to add http prefix.

Also, how about changing the message to something more actionable, like http.%s requires two arguments, but %d were provided. Please adjust the input.

Copy link
Contributor

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

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

Looks like the new code is not correctly formatted, @ArjitGopher could you run the linter and fix the related issues, please?

Beyond that, I only have a small suggestion because I think these functions are usually called like http.get(...), as suggested in the official docs, and in any case, it's always good idea to disambiguate.

Other than that, it looks good! 👍🏻
cc/ @olegbespalov

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

I do agree with what @joanlopez said, also have a minor suggestion for the actual message

Comment on lines 64 to 68
validateArgCount := func(methodName string,args ...sobek.Value){
if len(args)>2 {
vu.State().Logger.Warningf("%s method has more than two arguments",methodName)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree with @joanlopez that it's better to add http prefix.

Also, how about changing the message to something more actionable, like http.%s requires two arguments, but %d were provided. Please adjust the input.

@olegbespalov olegbespalov added the awaiting user waiting for user to respond label Oct 11, 2024
@olegbespalov
Copy link
Contributor

Hi @ArjitGopher. Out of curiosity, are you going to apply feedback, or you have no capacity of doing that?

@ArjitGopher
Copy link
Author

Hi @olegbespalov I agree with the suggestions and will add them today, Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting user waiting for user to respond
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants