rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.93k stars 417 forks source link

Add refactoring check suggesting to use Enum.count/2 if applicable #1022

Closed frerich closed 1 year ago

frerich commented 1 year ago

This extends Credo such that it looks for use cases for Enum.count/2. Alas, I only learned about https://github.com/rrrene/credo-proposals after creating these commits so this PR is 'out of the blue'. 😔 However, I hope that it's not too controversial since the proposed refactoring is much like the one for Enum.map_join/3.

Using Enum.count/2 instead of composing Enum.filter/2 with Enum.count/1 is preferable since

  1. Enum.count/2 requires just a single pass, so it's a little more efficient
  2. It's less code

The check ID EX4030 was determined by checking the other IDs and simply picking the next larger value; the largest check ID for refactoring checks appears to be EX4029, so that's how I ended up with EX4030. 😊

The test suite exercises various ways of composing Enum.filter/2 and Enum.count/1, making sure that uses of the |> operator don't confuse the check and that e.g. using Enum.filter/2 with Enum.count/2 does not trigger.

frerich commented 1 year ago

@rrrene I believe the 'reproducing test-case detector' check is a false negative: it would pass if the test/test_if_tests_fail_after_resetting_lib.sh script would pass --no-overlay to the git checkout invocation.

As it is, git checkout is used in "overlay" mode, i.e. checking out the lib/ directory as it is in the master branch will not remove any files. Thus, the new lib/credo/check/refactor/filter_count.ex added by this PR does not get removed and consequently the new test/credo/check/refactor/filter_count_test.exs test won't fail.

rrrene commented 1 year ago

@frerich This is by design. The mentioned action is used to see if a bugfix is coming with a reproducing test.

The fail is not relevant for this PR.

rrrene commented 1 year ago

This is part of Credo v1.7.0-rc.1 :+1: