Castone22
commented
5 years ago We need to be a little cautious with this.
Rubocop if nothing if not HIGHLY opinionated and sometimes it actually suggests doing stuff that breaks the ruby styleguide (just look what happens if you try to use method missing to make a facade, it suggests solutions for refactor that actively change the outward behavior of your code.)
If we want to rely on it more heavily, we need to ensure we've got a good profile configured for it.
Current problems it detects in the oz codebase itself tend to be:
-
ABC Complexity: This refers to assignment, branching and conditions. It basically means that your method is probably breaking the single responsibility principle. Router's
get_routes_betweenmethod falsely throws this because it assigns a lot of local variables, uses a loop and has multiple exits, including a guard statement. (This could theoretically be fixed but requires a pretty big rewrite, not refactor, of how router works.) -
Method Has too many lines: This is a FUN one. Rubocop simultaneously likes to yell about this while ALSO telling you that guard statements need an extra line after them among other things that force a longer method. It also discourages you from saving variables to give the results of various method calls a more.
-
Too many instance variables: Right then. So don't use model classes at all?
Currently there are a vast amount of rubocop errors and warnings that are thrown (over 1k). This should be addressed to gain adherence to proper code formatting and current Ruby suggestions where possible. If there are specific smells that we decide to avoid, we need to create a rubocop config file to ignore them.