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.

Sign up for GitHub

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

Jump to bottom

add automotive data #692

Closed
wants to merge 7 commits into from
Closed

add automotive data #692

wants to merge 7 commits into from

Conversation

ivanovaleksey
Copy link

@ivanovaleksey ivanovaleksey commented Sep 14, 2016 •
edited
Loading

#254 seems to be stale.

I fetched commits from the PR, fix specs and some methods.

TODO

  • update README

@ivanovaleksey ivanovaleksey mentioned this pull request Sep 14, 2016
Closed
b264
b264 reviewed Sep 16, 2016
View reviewed changes
lib/faker/auto.rb Outdated
end

def model(make = '')
return fetch('auto.models_by_make').values.flatten.sample if make.empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest not to shadow the make method with a local variable, can it be renamed? _make maybe

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I didn't noticed it 😞

Copy link
Author

Choose a reason for hiding this comment

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

I have force pushed the changes.

@rui-ribeiro
Copy link

Why don´t you extend Faker::Vehicle?

@ivanovaleksey
Copy link
Author

Good question. As you can see I just fetch existing #254 and fix some code and specs.
The original PR is dated by Sept. 2014 and Faker::Vehicle is much newer (it's added in Jun. 2016).
To be honest I didn't see Faker::Vehicle. I will try to extend it.

@ivanovaleksey
Copy link
Author

@vbrazo unfortunately, I don't have a time for that.
Feel free to change assignee.

@vbrazo vbrazo force-pushed the master branch 5 times, most recently from a359def to a5d7731 Compare May 22, 2018 21:15
@vbrazo
Copy link
Member

vbrazo commented May 27, 2018

@aamarill @mrstebo hey guys, would you be interested in working on this PR?

@mrstebo
Copy link
Contributor

mrstebo commented May 27, 2018

Sure, I'll take a look at it tomorrow

@vbrazo vbrazo assigned vbrazo and unassigned vbrazo May 27, 2018
@vbrazo
Copy link
Member

vbrazo commented May 27, 2018

Ok cool

@aamarill
Copy link

@vbrazo sorry, I won't be able to help with this any time soon. 😕 Thanks for keeping me mind though 😄

@mrstebo
Copy link
Contributor

mrstebo commented May 28, 2018

I'm on it! 😎

@mrstebo mrstebo mentioned this pull request May 29, 2018
Merged
@vbrazo vbrazo closed this May 29, 2018
@vbrazo
Copy link
Member

vbrazo commented May 30, 2018

Thanks for the hard work @ivanovaleksey

We are working in this feature in another PR #1260

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b264 b264 b264 left review comments

Assignees
No one assigned
Labels
💡 Issue: Feature Request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ivanovaleksey @rui-ribeiro @vbrazo @mrstebo @aamarill @b264 @haruska