diaspora-federation icon indicating copy to clipboard operation
diaspora-federation copied to clipboard

Small fixes

Open dimaursu opened this issue 10 years ago • 5 comments

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.

dimaursu avatar Jan 29 '15 02:01 dimaursu

Coverage Status

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

coveralls avatar Jan 29 '15 02:01 coveralls

My comments in https://github.com/diaspora/diaspora/pull/5561 apply here too, Rubocops default config is a nightmare.

jhass avatar Jan 29 '15 03:01 jhass

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 avatar Jan 29 '15 12:01 dimaursu

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

dimaursu avatar Jan 29 '15 12:01 dimaursu

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.

jhass avatar Jan 29 '15 13:01 jhass