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

Small fixes #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Small fixes #9

wants to merge 4 commits into from

Conversation

dimaursu
Copy link

Although the pull-request is big, there are not radical changes. The most important one are removal of several methods, and covert those in attr_reader's, and the commit that deals with providing more information on failure.

This:
ArgumentError:
       316 is not a valid author_id

is better than this:
ArgumentError:
    ArgumentError
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.86% when pulling e32bffd on dimaursu:master into 5c308b6 on Raven24:master.

@jhass
Copy link
Collaborator

jhass commented Jan 29, 2015

My comments in diaspora/diaspora#5561 apply here too, Rubocops default config is a nightmare.

@dimaursu
Copy link
Author

https://travis-ci.org/Raven24/diaspora-federation/jobs/48712715 Would require: false on rubocop help here?

@jhass the alternative doesn't warm me either, i.e. checking the styleguide by hand. I find most of the rubocop defaults sensible, tbh. Like the fail vs raise stuff - when you intent to really raise an error, and catch it somewhere later, it's fine - but for most of the time you just intend to stop the execution with an error message, and fail helps to convey your intent. Even if fail and raise are aliases, they should be use to express what you actually mean.

Some of the settings are not reasonable to me, like the 80 char line lenght - I have to "uglify" the code, just to fit in this rule, and this tradeoff I'm not willing to make.

Some of the rules helped me catch bugs or small issues, and it definitely helped me simplify the code a bit with good suggestions. It's definitely a compromise I'm willing to make.

@dimaursu
Copy link
Author

It would be interesting to enable HoundCI, to see what's the difference.

@jhass
Copy link
Collaborator

jhass commented Jan 29, 2015

No, require: false just excludes the gem from Bundler.require. While it looks like a temporary failure, you want to put it into a group that's excluded in CI, like development usually.

I see fail very rarely used in real world code. And raising an exception with the intent to rescue it later is doing control flow with exceptions, which you should avoid. Exceptions are for the exceptional, that means very rare, cases. Thus raise means to me already what you claim fail means and I have no keyword for what you claim raise means (when talking about intent). And that results in having to know two keywords for doing the same thing.

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.

3 participants