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

Rubocop + fixes #924

Merged
merged 2 commits into from May 15, 2018
Merged

Rubocop + fixes #924

merged 2 commits into from May 15, 2018

Conversation

stephengroat
Copy link
Contributor

@stephengroat stephengroat commented May 20, 2017

enables rubocop on the entire repo, disabled some cops (see .rubocop.yml)

need some help on the last few (if possible)

@vbrazo
Copy link
Member

vbrazo commented May 14, 2018

@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 👍

@stephengroat
Copy link
Contributor Author

@vbrazo rebased the commit against master and squashed so there's less clutter to look through

still has some errors
@vbrazo
Copy link
Member

vbrazo commented May 15, 2018

@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?

@stephengroat
Copy link
Contributor Author

@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 .rubocop.yml file, there i'm actually just disabling a bunch of cops (the Enabled: false syntax is annoying). This is the complete list of cops, all of the rest are enabled. as you can see, it's hard to just enable 1 or 2 cops on the list.

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
Copy link
Member

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

Copy link
Member

@vbrazo vbrazo May 15, 2018

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.

Copy link
Contributor Author

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 }
Copy link
Member

Choose a reason for hiding this comment

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

👍

@vbrazo vbrazo merged commit 448cc4f into faker-ruby:master May 15, 2018
@stephengroat stephengroat deleted the rubocop branch May 15, 2018 06:16
@stephengroat
Copy link
Contributor Author

Just as an fyi, I added rubocop to the default task

You'd have to remove the test arg in Travis to have rubocop run in Travis

@vbrazo
Copy link
Member

vbrazo commented May 15, 2018

@stephengroat I'll remove it. Thanks for the heads up 💯

@vbrazo vbrazo self-requested a review July 19, 2018 01:29
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Rubocop + fixes

still has some errors

* Readd travis script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants