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

Added rubocop.yml and corrected simple style warnings #283

Closed
wants to merge 1 commit into from

Conversation

iainbeeston
Copy link
Contributor

I'd like to enable HoundCI for json-schema. It works using rubocop to warn of style violations on pull requests. Before we can do that, we need to establish a rubocop style definition for the project and correct existing style violations.

This PR adds rubocop and houndci config, and corrects inconsistencies in the code style used throughout the project. Where two different styles were in use (in different files) I've tried to use whichever style is most common throughout the codebase.

@RST-J
Copy link
Contributor

RST-J commented Jan 7, 2016

I also vote for adding rubocop as style checker. We use that in our company, too.
As a reference we use the ruby styleguide .

I haven't read the whole config yet but I remember that rubocop by default has very strict cops regarding method and class size and complexity. So what exactly do want to have as limits in this respect? I mean as a goal, by now we won't score any acceptable values I guess.

@iainbeeston
Copy link
Contributor Author

The rubocop config I've added uses the default Hound CI as the base, and then I've customised it to the norms in this project.

All of the complexity and class/method length metrics are disabled at the moment. I've corrected every (reasonable) rubocop error, so that in this branch, there are no rubocop warnings at all.

* Largely based on the default styles for houndci
@RST-J
Copy link
Contributor

RST-J commented Jan 8, 2016

Ok, I'll try to find time to investigate the config some time soon - I think we'll go for it, the question is if everyone agrees about the defined style.
The one thing that immediately came to my mind - no spaces in hash literals - is currently in my favour 😀

@iainbeeston
Copy link
Contributor Author

With the style config, I've tried to make it enforce the most common style throughout the codebase. For example, we use double-quotes more than single, so that became the enforced style. So hopefully it's representative, and just makes things more consistent. However I'd be more than happy to adjust it to suit tastes, or make it more strict

@RST-J
Copy link
Contributor

RST-J commented Jan 8, 2016

I don't intend to provoke hard discussions, probably its all fine already or there are only minor things, I don't know.
But I think at least we four, i.e. also @hoxworth and @pd, should agree upon the config when we introduce it.

@iainbeeston
Copy link
Contributor Author

Sure, that sounds sensible. I'm not in a rush.

collect!: map!
find: detect
find_all: select
reduce: inject
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer reduce over inject here.

@RST-J
Copy link
Contributor

RST-J commented Nov 8, 2016

Do we want to go on with this? I'd say yes. The other two would have had enough time to react, I'd say.

@bolshakov
Copy link
Contributor

It should be safe to close this PR, rubocop was added here #480

@ekohl ekohl closed this Jul 9, 2024
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.

4 participants