Skip to content

Conversation

@rohitpaulk
Copy link
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
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
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
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