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 integration? #27

Closed
ShockwaveNN opened this issue Mar 5, 2018 · 6 comments
Closed

Rubocop integration? #27

ShockwaveNN opened this issue Mar 5, 2018 · 6 comments

Comments

@ShockwaveNN
Copy link
Contributor

Hi there, while filing some bug for this project I noticed what code style of this project is not in great shape.
I can help with integration of RuboCop into project and fixing current code style issues if authors are agree with me.

@kou
Copy link
Member

kou commented Mar 5, 2018

I don't like RuboCop but it's OK to me if we start from a loose configuration that doesn't report anything for the current code base.

@ShockwaveNN
Copy link
Contributor Author

ShockwaveNN commented Mar 5, 2018

@kou I think currently RuboCop generate pretty interesting enhancements for current codebase, at least for better readability.
F.e. replacing

csv/lib/csv.rb

Line 1180 in 120b115

if part.end_with?(@quote_char) && part.count(@quote_char) % 2 != 0

to
if part.end_with?(@quote_char) && part.count(@quote_char).odd?
which is better looking for me.

Or duplicate private on

private

and
private

And since gem has spec.required_ruby_version = ">= 2.4.0" we can remove redundant encoding: UTF-8 in some test files.

And so on

@kou
Copy link
Member

kou commented Mar 5, 2018

Do you mean that "we should start from an optimized configuration"?
It your answer is yes, I object this proposal.

@yuki24
Copy link
Member

yuki24 commented Mar 5, 2018

While I understand the feelings about the code styles in the repo, Rubocop is very opinionated, which makes it extremely difficult to get everyone on the same page. I took a quick look at the source code and found it very readable and easy to understand even without Robocop. Personally, I don't see why we should incorporate Rubocop into this project.

That being said, the two points you brought up are totally legitimate. I'm not sure about the odd? one (odd? is definitely more readable, but x % 2 != 0 is not that bad), but I'd happily merge a PR that removes the duplicate private in if you could send one.

@mrkn
Copy link
Member

mrkn commented Mar 6, 2018

@ShockwaveNN As I described in the recent commit b8766d8, we don't use RuboCop in this small repository.

Please file your pull-request in your own coding style with some adjustments to fit the lines around of your change.

@ShockwaveNN
Copy link
Contributor Author

Ok I thing I got official statement about RuboCop from dev team and seems this issue could be closed.
I don't think I can adapt RuboCop config file to code style of current repository , or if I could I don't think workflow would be effective.

@yuki24 I create PR about duplicate private #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants