Skip to content

Add linter#130

Merged
f2prateek merged 36 commits into
segmentio:masterfrom
rohitpaulk:add-linter
Nov 29, 2017
Merged

Add linter#130
f2prateek merged 36 commits into
segmentio:masterfrom
rohitpaulk:add-linter

Conversation

@rohitpaulk

Copy link
Copy Markdown
Collaborator

This PR adds a lint step (using rubocop) to the CI process.

All trivial violations have been fixed, and a few not-so-easy to fix items have been left as pending in rubocop_todo.yml.

The only pending item I see as wildly abnormal is the line length - the high numbers are due to comments such as this one. I couldn't recognize the format used for comments - it had special markers like private:, public:, none that I've seen used in RDoc or YARD markup. If this comment format isn't intentional, I can open another PR to rewrite them in YARD markup.

I've tried to split changes into small commits so that they're easier to review.

@f2prateek

Copy link
Copy Markdown
Contributor

Awesome, this looks great. Let's merge it to unblock the other PRs.

The only pending item I see as wildly abnormal is the line length - the high numbers are due to comments such as this one. I couldn't recognize the format used for comments - it had special markers like private:, public:, none that I've seen used in RDoc or YARD markup. If this comment format isn't intentional, I can open another PR to rewrite them in YARD markup.

Yeah let's fix those! I don't think it was formatted for a tool, we can definitely switch to using RDoc or YARD here. Is there a reason to prefer one over the other? RDoc does seem more widely used because it's included with Ruby

@f2prateek f2prateek merged commit 3c44ee9 into segmentio:master Nov 29, 2017
@f2prateek

Copy link
Copy Markdown
Contributor

Also as a follow up here - are there any tools that will automate this? e.g. we use https://github.com/prettier/prettier and https://github.com/google/google-java-format in other languages - is there something similar for Ruby?

@f2prateek

Copy link
Copy Markdown
Contributor

If we can't automate it, a static analysis like this is fine as well - maybe we could add it to the CI to enforce it?

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.

2 participants