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

Fixes Metrics/CyclomaticComplexity #8149

Merged
merged 3 commits into from Jun 18, 2020
Merged

Conversation

marcandre
Copy link
Contributor

This PR fixes CyclomaticComplexity for ||=, &&=, &.

Blocks are tricky as some blocks clearly add to the complexity

hash.transform_values { ... } # => should adds 1

Other builtin methods don't actually add to the cyclomatic
complexity, like tap, yield_self, class_eval, ...

obj.tap { ... } # => should no count

As @jonas054 pointed out, others methods in general can not be known.

This PR takes the conservative approach and counts only known iterating/conditional methods and ignores the rest.

If the default for CyclomaticComplexity remains at 6, there would be 34 new todos in the main gem.
If we bump it at 7, there are 5 new todos, 2 can be removed.
Bumping to 8 would have 1 new todo and 4 that would be removed.

I'm proposing to bump it to 7.

@marcandre marcandre marked this pull request as draft June 12, 2020 21:11
@marcandre marcandre assigned marcandre and unassigned marcandre Jun 12, 2020
@marcandre marcandre marked this pull request as ready for review June 16, 2020 06:23
@marcandre
Copy link
Contributor Author

Got a 👍 from @jonas054, do I get a go from @bbatsov ?

@marcandre
Copy link
Contributor Author

marcandre commented Jun 18, 2020

I was about to merge this and noticed that while this was counting ary.map { |x| to_s } it wasn't counting ary.map(&:to_s). PR fixed. 3 additional todos in the code. An example going from 6 to 9:

# cop/style/empty_case_condition.rb
        def on_case(case_node)
          return if case_node.condition
          return if case_node.when_branches.any? do |when_branch|
            when_branch.each_descendant.any?(&:return_type?)
          end

          if (else_branch = case_node.else_branch)
            return if else_branch.return_type? ||
                      else_branch.each_descendant.any?(&:return_type?)
          end

          add_offense(case_node, location: :keyword)
        end

I'm ok with this being counted as "complex". I couldn't help it and refactored it 😅. tangential: maybe Node#each_descendant should have an option include_self: false. Also CaseNode#branch_bodies might be helpful.

@marcandre marcandre force-pushed the cyclo branch 2 times, most recently from 670024f to ec5aa21 Compare June 18, 2020 18:02
@tonobo
Copy link

tonobo commented Jun 23, 2020

Howdy guys, you've recently introduced "pending" cops to reduce the risk of breaking pipelines and however you're updating the cyclo calculation that's my humor. 😄

I don't want to criticize the decision about the new default value (and how it's be fallen), but although it introduces "only" 5 changes for you guys, we have dozens of pipelines failing due to that change. And increasing the value even further to have less work is no option for us.

What do you think? How can we prevent such changes to be breaking changes? (maybe cop versioning)

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 23, 2020

Howdy guys, you've recently introduced "pending" cops to reduce the risk of breaking pipelines and however you're updating the cyclo calculation that's my humor. 😄

I guess the problem here is that the changes were considered a bug-fixes, not a behaviour change (although they do change the behaviour). The new default itself is not a problem in itself, as it was actually relaxed, so for me the problem is how to communicate better bug-fixes that would probably result in broken pipelines. Sometimes we're aware that a breakage is likely to occur (e.g. in this case), but some things would be a surprise even to us.

I'm not sure what's the best course of action here.

@marcandre
Copy link
Contributor Author

marcandre commented Jun 23, 2020

@tonobo: Really sorry it's causing you trouble. I understand it can be frustrating to have code that previously was ok to now be causing offenses. I had hoped that this change would land in v1.

Are you aware that rubocop -a --disable-uncorrectable will add the disabling comments to the newly offensive methods? It could be an easy solution. I'll add a note in the Changelog to that effect, and I probably should also put it in "Breaking changes" section so users are warned.

Heads up: AbcSize will also be impacted in the next version (v1.0). It would be a good idea to update PerceivedComplexity too. If they ever change again it shouldn't be in such a drastic fashion and we should definitely consider a preference setting since we'll be in v1.

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

3 participants