-
Notifications
You must be signed in to change notification settings - Fork 605
Value on api call / curl example fix #193
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #193 +/- ##
=========================================
Coverage 79.07% 79.07%
Complexity 178 178
=========================================
Files 7 7
Lines 626 626
=========================================
Hits 495 495
Misses 131 131
Continue to review full report at Codecov.
|
| case 'in': | ||
| $attributeData['description'][] = Description::parse($rule)->with($this->fancyImplode($parameters, ', ', ' or '))->getDescription(); | ||
| $attributeData['value'] = $faker->randomElement($parameters); | ||
| // $attributeData['value'] = $faker->randomElement($parameters); |
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.
Hi @prodigy7 thanks for the PR. I'm not sure I understand why we're always choosing the first parameter. Maybe you should add a code comment for anyone who sees it in the future.
I get that we're using a default value, but what if the user doesn't want to use a default value? I'm thinking we can find another way of doing this. Maybe an annotation (// @apidocs default-value)
Also, please don't comment out old code. Delete it. Let Git do the tracking. 😀
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.
I'm not very familiar with cURL, but the rest of the PR looks okay. So, if you attend to this, I can merge this PR. Also, can you update the documentation to mention the env variables it uses?
|
Looks like this is abandoned. Integrated the changes into #328 |
See issue #191 points 1, 2 and 4 and note at end of issue