-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Rubocop + fixes #924
Rubocop + fixes #924
Conversation
@stephengroat Thanks for contributing 🥇 I love Rubocop and really like the idea of adding this gem to the Faker project. I'll make sure to review this PR in the next days 👍 |
@vbrazo rebased the commit against master and squashed so there's less clutter to look through |
still has some errors
@stephengroat Awesome!! I love it. ❤️ 💓 Do you mind if we start with just a few Rubocop configs? I think it would be a great idea if we could split the changes in 2 PRs. What do you think? |
@vbrazo i understand trying to split it up, but rubocop is kinda an all or nothing event. plus, being able to have a single commit to get through the git blame is nice. if you look at the could you describe exactly what you're trying to get from the split? maybe there's something else i could do? |
.travis.yml
Outdated
@@ -7,7 +7,6 @@ rvm: | |||
- 2.3.4 | |||
- 2.4.1 | |||
- ruby-head | |||
script: bundle exec rake test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we removing this line? @stephengroat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should actually add the rubocop script instead of removing this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, i have a gem that removes the defaults from .travis.yml
automatically.
i can add it back in if needed. rubocop
is now integrated into the default rake
task
def numerify(number_string) | ||
number_string.sub(/#/) { (rand(9)+1).to_s }.gsub(/#/) { rand(10).to_s } | ||
number_string.sub(/#/) { rand(1..9).to_s }.gsub(/#/) { rand(10).to_s } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Just as an fyi, I added rubocop to the default task You'd have to remove the |
@stephengroat I'll remove it. Thanks for the heads up 💯 |
* Rubocop + fixes still has some errors * Readd travis script
enables rubocop on the entire repo, disabled some cops (see
.rubocop.yml
)need some help on the last few (if possible)