Skip to content

Remove the ffaker sample data dependency (revised)#2163

Merged
cbrunsdon merged 1 commit intosolidusio:masterfrom
swcraig:finish-clarke-pr-2140
Aug 22, 2017
Merged

Remove the ffaker sample data dependency (revised)#2163
cbrunsdon merged 1 commit intosolidusio:masterfrom
swcraig:finish-clarke-pr-2140

Conversation

@swcraig
Copy link
Copy Markdown
Contributor

@swcraig swcraig commented Aug 17, 2017

Addresses comments brought up in @cbrunsdon's original PR.

Original PR description

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.

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.
@swcraig
Copy link
Copy Markdown
Contributor Author

swcraig commented Aug 17, 2017

#2140 (comment)

@tvdeyen I didn't change the name arrays to use %w syntax because the other arrays in this file use multi-word values and I felt that consistency in the file would be favoured. Instead I spread the arrays over multiple lines when they grew past 80 characters. Let me know if you want me to revert the arrays to sit on single lines again (or whatever solution you find reasonable).

Copy link
Copy Markdown
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.

Thanks. I'totally fine with the array syntax 👌

Copy link
Copy Markdown
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.

Good by me, thanks for finishing this off sebastian

Copy link
Copy Markdown
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Thanks @swcraig, nice work!

@mtomov
Copy link
Copy Markdown
Contributor

mtomov commented Aug 18, 2017

Thanks!

@cbrunsdon cbrunsdon merged commit 53ee38a into solidusio:master Aug 22, 2017
@jhawthorn jhawthorn mentioned this pull request Oct 31, 2017
fastjames pushed a commit to geminimvp/solidus_subscriptions that referenced this pull request Aug 1, 2018
ffaker was removed as a runtime dependency of Solidus. As of right now,
this removal is only on Solidus master (but it is slated for v2.4).

Even though this extension does not use ffaker directly, Solidus
complains that its factories require the library and so the tests fail
without this.

PR removing ffaker from Solidus:
solidusio/solidus#2163
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.

5 participants