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

Add HTTP status codes generator #1821

Merged
merged 9 commits into from Jun 18, 2020
Merged

Add HTTP status codes generator #1821

merged 9 commits into from Jun 18, 2020

Conversation

willianveiga
Copy link
Contributor

Description:

Add HTTP status codes generator:

Faker::Internet::HTTP.status_code # 302

Faker::Internet::HTTP.information_status_code # 101

Faker::Internet::HTTP.successful_status_code # 200

Faker::Internet::HTTP.redirect_status_code # 301

Faker::Internet::HTTP.client_error_status_code # 404

Faker::Internet::HTTP.server_error_status_code # 500

Copy link
Member

@vbrazo vbrazo left a 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.

@vbrazo
Copy link
Member

vbrazo commented Oct 30, 2019

I wonder if we really need a new object. We could simply do Faker::Internet.http_status_code to get a status code. Thoughts?

@vbrazo
Copy link
Member

vbrazo commented Oct 30, 2019

Another idea would be passing the status code error message in the parameter instead of having the interface methods. I meanFaker::Internet.http_status_code(status_error: :client_error). If you don't pass anything, you'd return a random status_error code.

Copy link
Member

@vbrazo vbrazo left a 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.

@willianveiga
Copy link
Contributor Author

willianveiga commented Nov 1, 2019

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.

@vbrazo
Copy link
Member

vbrazo commented Nov 2, 2019

@faker-ruby/core-contributors thoughts?

@jremes-foss
Copy link
Contributor

jremes-foss commented Nov 2, 2019

@vbrazo @faker-ruby/core-contributors Add a new class. The separation of concerns is a valid point.

# @example
# Faker::Internet::HTTP.status_code(group: :server_error) #=> 502
#
# @faker.version 2.7.0
Copy link
Contributor

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

@jremes-foss
Copy link
Contributor

Hi. Looks like the failing build was caused by RuboCop:

lib/faker/default/internet_http.rb:19:1: C: Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body beginning.

Please fix.

@@ -0,0 +1,11 @@
# Faker::Internet::HTTP

Available since version 2.7.0.
Copy link
Contributor Author

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

Copy link
Contributor

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 <koic.ito@gmail.com>
@willianveiga willianveiga requested review from koic and vbrazo June 14, 2020 21:44
Copy link
Member

@psibi psibi left a comment

Choose a reason for hiding this comment

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

Minor comment

@koic koic merged commit 2242a97 into faker-ruby:master Jun 18, 2020
@koic
Copy link
Member

koic commented Jun 18, 2020

Thanks!

@willianveiga willianveiga deleted the feature/add-http-status-codes-generator branch June 18, 2020 09:23
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.

None yet

7 participants