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

Support IgnoredMethod configuration in method complexity cops #7823

Merged

Conversation

Drenmi
Copy link
Collaborator

@Drenmi Drenmi commented Mar 26, 2020

Rationale:

At work we make extensive use of functional objects to encapsulate business transactions. These objects will have a single public method #call, which stacks all the edge-cases as guard clauses towards the top.

It usually looks something like:

def call
  # Edge cases.
  #
  return error(post.errors.full_messages.to_sentence) unless post.valid?
  return error("Author har been blocked from posting.") if author.blocked?
  return error("Author doesn't have an active subscription.") unless author.current_subscription.active?

  # Side effects.
  #
  create_post!
  send_notifications!

  # Success!
  #
  success(post: post)
end

We find this makes it very easy to understand what's going on, business transactions become composable and easy to test.

This almost always leads to Metrics cops violations for #call. However, the method is perfectly fine. With this layout, more complex business transactions aren't necessarily harder to grok than simpler ones.

Others seem to report the same problem.

What is this change?

This adds support for IgnoredMethods to Metrics/AbcSize, Metrics/CyclomaticComplexity, and Metrics/PerceivedComplexity.

Side note:

We should probably standardize the configuration option name before releasing 1.0. I used IgnoredMethods because we have an existing mixin for it, but I see other cops roll their own implementations and names.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@Drenmi Drenmi requested review from koic, Darhazer and pocke March 26, 2020 03:34
CHANGELOG.md Outdated Show resolved Hide resolved
@Drenmi Drenmi force-pushed the feature/method-complexity-ignored-methods branch from 9a0eecb to 9f87af9 Compare March 26, 2020 06:19
@bbatsov bbatsov merged commit 3405fe6 into rubocop:master Mar 26, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 26, 2020

We should probably standardize the configuration option name before releasing 1.0. I used IgnoredMethods because we have an existing mixin for it, but I see other cops roll their own implementations and names.

Agreed.

@Drenmi Drenmi deleted the feature/method-complexity-ignored-methods branch March 27, 2020 02:55
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.

None yet

4 participants