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 Faker::Commerce.rating #2808

Closed
wants to merge 2 commits into from
Closed

Conversation

bradly
Copy link

@bradly bradly commented Aug 11, 2023

Motivation / Background

This Pull Request has been created because I needed to generate ratings for a product.

Additional information

My use case was for a rating for a product so I put this in Fake::Commerce, but there are other generators that could have a ratings as well: App, Book, Game, Movie, Restaurant, etc..

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug, refactor something, or add a feature.
  • Tests and Rubocop are passing before submitting your proposed changes.

If you're proposing a new generator:

@bradly bradly force-pushed the commerce-rating branch 3 times, most recently from 69f0fa7 to 85ab1f7 Compare August 11, 2023 19:30
@bradly bradly changed the title Adding Faker::Commerce.rating Add Faker::Commerce.rating Aug 11, 2023
doc/default/commerce.md Outdated Show resolved Hide resolved
# Faker::Commerce.rating(range: 0..10.0, decimals: 2) #=> 8.92
#
# @faker.version next
def rating(range: 0..5.0, decimals: 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of adding a check here for the decimals to be within a reasonable limit? and checking if the decimals can't be negative?

@@ -8,7 +8,7 @@ en:
product: [Chair, Car, Computer, Gloves, Pants, Shirt, Table, Shoes, Hat, Plate, Knife, Bottle, Coat, Lamp, Keyboard, Bag, Bench, Clock, Watch, Wallet]
promotion_code:
adjective: ['Amazing', 'Awesome', 'Cool', 'Good', 'Great', 'Incredible', 'Killer', 'Premium', 'Special', 'Stellar', 'Sweet']
noun: ['Code', 'Deal', 'Discount', 'Price', 'Promo', 'Promotion', 'Sale', 'Savings']
noun: ['Code', 'Deal', 'Discount', 'Price', 'Rating', 'Promo', 'Promotion', 'Sale', 'Savings']
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have a Japanese commerce locale. Could you update that as well?

test/faker/default/test_faker_commerce.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@stefannibrasil stefannibrasil left a comment

Choose a reason for hiding this comment

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

Thank you for submitting the changes @bradly

Could you add the following tests as well:

  • Verify that as_string works with other arguments. For example: Faker::Commerce.rating(range: 0..10, decimals: 2, as_string: false) returns a string for me.
  • Could you clarify more about the use case for as_string? Is that something the user can parse on their own if they need it? I'm a bit unsure we should have the responsibility of converting the Rating.
  • What do you think of adding upper limits to both range and decimals? People could enter high numbers for them, which would crash the generator.
  • Have you considered returning an ArgumentError for when the decimals are not positive? That way, the user can identify what the error is and fix it, then try again. If we just ignore it, they won't know what happened. We can also handle range and decimals as ArgumentError when they exceed the upper limit. What upper limits do you suggest for them?

Thanks!

@stefannibrasil
Copy link
Contributor

hi @bradly are you still interested in working on this one? Thanks!

@stefannibrasil
Copy link
Contributor

Hi @bradly I will close this since it hasn't been active for a couple of months. Feel free to re-open and rebase with main if you're still planning on working on this. Thanks!

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

3 participants