Remove the ffaker sample data dependency #2140
Conversation
sample/db/samples/addresses.rb
Outdated
| state: new_york, | ||
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| phone: phone_numbers.sample) |
There was a problem hiding this comment.
Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
sample/db/samples/addresses.rb
Outdated
| state: new_york, | ||
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| phone: phone_numbers.sample) |
There was a problem hiding this comment.
Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
sample/db/samples/addresses.rb
Outdated
| @@ -1,26 +1,35 @@ | |||
| united_states = Spree::Country.find_by!(iso: "US") | |||
| new_york = Spree::State.find_by!(name: "New York") | |||
|
|
|||
| require 'pry' | |||
| binding.pry | |||
There was a problem hiding this comment.
Remove debugger entry point binding.pry.
sample/db/samples/addresses.rb
Outdated
| state: new_york, | ||
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| phone: phone_numbers.sample) |
There was a problem hiding this comment.
Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
sample/db/samples/addresses.rb
Outdated
| state: new_york, | ||
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| phone: phone_numbers.sample) |
There was a problem hiding this comment.
Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
sample/db/samples/addresses.rb
Outdated
| @@ -1,26 +1,35 @@ | |||
| united_states = Spree::Country.find_by!(iso: "US") | |||
| new_york = Spree::State.find_by!(name: "New York") | |||
|
|
|||
| require 'pry' | |||
| binding.pry | |||
There was a problem hiding this comment.
Remove debugger entry point binding.pry.
cbrunsdon
force-pushed
the
remove_ffaker_dependency
branch
from
7c3117f to
8a02a5c
Compare
sample/db/samples/addresses.rb
Outdated
| state: new_york, | ||
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| phone: phone_numbers.sample) |
There was a problem hiding this comment.
Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
sample/db/samples/addresses.rb
Outdated
| state: new_york, | ||
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| phone: phone_numbers.sample) |
There was a problem hiding this comment.
Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
Continues the idea of solidusio#2082, this removes the dependency of ffaker but also yanks it out of the sample requirement. I generated a small data set with 15.times.map { FFaker::Name.first_name } etc.
cbrunsdon
force-pushed
the
remove_ffaker_dependency
branch
from
8a02a5c to
433ef54
Compare
There was a problem hiding this comment.
Nice 👏
... thats pretty reasonable in exchange for removing ffaker as a useless runtime dependency in prod.
We only require ffaker in one place.
Why not add a guard around the require 'ffaker' line in core/lib/spree/testing_support/sequences.rb?
begin
require 'ffaker'
rescue LoadError
abort "Solidus factories require FFaker. Please add `ffaker` to your `Gemfile`"
end| @@ -35,6 +35,7 @@ | |||
| gem 'with_model' | |||
| gem 'rspec_junit_formatter' | |||
| gem 'rails-controller-testing' | |||
| gem 'ffaker' | |||
There was a problem hiding this comment.
Could we add require: false here? We don't need to require ffaker every time we boot the dummy apps.
| zipcode: 16_804, | ||
| country: united_states, | ||
| phone: FFaker::PhoneNumber.phone_number) | ||
| first_names = ["Sterling", "Jennette", "Salome", "Lyla", "Lola", "Cheree", "Hettie", "Barbie", "Amelia", "Marceline", "Keeley", "Mi", "Karon", "Jessika", "Emmy"] |
There was a problem hiding this comment.
Personally I prefer the %w(Sterling Jennette) syntax for arrays of strings. Also having a new line for each word is far better readable, but this should not block this PR.
|
We probably want to add a changelog entry |
|
Closed in favor of #2163 |
See this PR solidusio/solidus#2140 for reference. This change also works with previous versions of solidus.
See this PR solidusio/solidus#2140 for reference. This change also works with previous versions of solidus.
ffaker was removed as a runtime dependency of Solidus. polyglot (which is a dependency of deface) is raising a LoadError when it tries to require ffaker, which is not defined. PR removing ffaker from Solidus: solidusio/solidus#2140
Continues the idea of #2082, this removes the dependency of ffaker but
also yanks it out of the sample requirement. I generated a small data
set with 15.times.map { FFaker::Name.first_name } etc.
It should be noted that there isn't any reasonable way to export development dependencies to people, and anyone trying to include our factories will get ffaker missing errors. I personally think thats pretty reasonable in exchange for removing ffaker as a useless runtime dependency in prod.