-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Add HTTP status codes generator #1821
Add HTTP status codes generator #1821
Conversation
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.
Please add /docs
and update the README
.
I wonder if we really need a new object. We could simply do |
Another idea would be passing the status code error message in the parameter instead of having the interface methods. I mean |
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 see that you want to add the Faker::Internet::HTTP
generator.
Ok no problem! Just one question for you:
Which other faker interface methods do you believe we could add to this new object?
If we only have this method inside this new generator, I'd definitely prefer to move this to Faker::Internet.http_status_code
. If you can tell me other useful methods, then we could see if we move on with this new object.
A new class would help me to separate concerns. HTTP is a different Internet concern. I can avoid breaking the Single Responsibility Principle and organize things better. We could have a new method returning HTTP status codes as symbols (:ok, :forbidden, etc.), for example. I think we can leave it as it is now and we can refactor it later in new PR, when I'll add this new method described above. What do you think, @vbrazo? Thank you very much. |
@faker-ruby/core-contributors thoughts? |
@vbrazo @faker-ruby/core-contributors Add a new class. The separation of concerns is a valid point. |
lib/faker/default/internet_http.rb
Outdated
# @example | ||
# Faker::Internet::HTTP.status_code(group: :server_error) #=> 502 | ||
# | ||
# @faker.version 2.7.0 |
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.
new generators should have version next
Hi. Looks like the failing build was caused by RuboCop:
Please fix. |
doc/internet/http.md
Outdated
@@ -0,0 +1,11 @@ | |||
# Faker::Internet::HTTP | |||
|
|||
Available since version 2.7.0. |
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.
Should I add next
here, @Zeragamba ? 🤔
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.
Yep
Co-authored-by: Koichi ITO <[email protected]>
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.
Minor comment
Thanks! |
Description:
Add HTTP status codes generator: