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

Remove ffaker #2339

Merged
merged 5 commits into from
Nov 1, 2017
Merged

Remove ffaker #2339

merged 5 commits into from
Nov 1, 2017

Conversation

jhawthorn
Copy link
Contributor

We are no longer using ffaker for sample data as of #2163. So we might as well remove the dependency entirely.

The main advantage of ffaker is being able to create data that looks to a human to be somewhat real. This isn't necessary or even desirable for the factories used by our specs, which just need different data. FactoryBot's sequences are totally adequate for these purposes.

This also removes a few "description" fields from factories of models where they were optional (factories should be creating minimal objects).

@jhawthorn jhawthorn requested a review from cbrunsdon October 31, 2017 20:35
@jhawthorn jhawthorn changed the title Remove more ffaker Remove ffaker Oct 31, 2017
@@ -15,7 +15,7 @@
address1 '10 Lovely Street'
address2 'Northwest'
city 'Herndon'
zipcode { FFaker::AddressUS.zip_code }
sequence(:zipcode, 10001) { |i| i.to_s }

Choose a reason for hiding this comment

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

Use underscores(_) as decimal mark and separate every 3 digits with them.

Copy link
Contributor Author

@jhawthorn jhawthorn Oct 31, 2017

Choose a reason for hiding this comment

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

no 😠

I have disabled this rule.

Copy link
Member

Choose a reason for hiding this comment

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

👌🏼

@@ -4,6 +4,6 @@
FactoryBot.define do
factory :promotion_code, class: 'Spree::PromotionCode' do
promotion
value { generate(:random_code) }
sequence(:value) {|i| "code#{i}" }

Choose a reason for hiding this comment

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

Space between { and | missing.

Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

You had me at "Remove".

Previously here we used TwitterCldr to generate zip codes, which we
replaced with FFaker. Neither of these generated accurate zipcodes for
the state they were in.

We might as well just generate codes from 10001 onwards (which should
mostly be valid codes in Manhattan).
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Nice

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks!

@jhawthorn jhawthorn merged commit 66a0ba0 into solidusio:master Nov 1, 2017
kennyadsl added a commit to solidusio-contrib/solidus_reviews that referenced this pull request Nov 3, 2017
They are no more present since solidusio/solidus#2339

Also, it follows the direction of the above PR and remove unnecessary
fields from some facotry to create minimal objects.
jontarg added a commit to jontarg/solidus_support that referenced this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants