Add regex filters #589

jsteel · 2017-06-16T19:24:09Z

Add the ability to use regex filters. The regexes should match against the files path relative to the current project, not the full path to root. For example, the file /Users/jon/project/app/foo/test.rb would match on a filter of "jon". I made the String and Array matchers follow that new rule as well.

Fixes #514

Fixes simplecov-ruby#514

PragTob

Wow, thank you very much 👍

This is a great PR, thanks a lot! :) 🎉

I've been looking, but I couldn't find anything to complain about :D Nice feature, good bug fix, solid testing.

I'm slightly afraid that it might involuntarily break something but I don't think so :D

If you wanted to be super awesome you could dust up the README section on filters a bit, adding the regex filter and Array filter but no worries I can also go around to doing this.

Let's see what the CI says.

I also asked my bunnies to find potential problems, they came up empty so far though.

PragTob · 2017-06-16T19:39:11Z

lib/simplecov/configuration.rb

-      elsif filter_argument.is_a?(Array)
-        SimpleCov::ArrayFilter.new(filter_argument)
+      elsif filter_argument
+        SimpleCov::Filter.class_for_argument(filter_argument).new(filter_argument)


Cool stuff! 👍

jsteel · 2017-06-19T13:12:29Z

No problem. I'll give the README some TLC today.

PragTob

Thanks a lot for the updates! Also thanks for fixing the ArrayFilter if I saw correctly :)

Had a couple of comments on that implementations, let's see where we get there :)

PragTob · 2017-06-20T07:41:21Z

README.md

+end
+```
+
+You can pass in an array containing any of the other filter types.


Thanks! 👍

PragTob · 2017-06-20T07:44:50Z

lib/simplecov/filter.rb

-    # Returns true if any of the file paths passed in the given array matches the string
-    # configured when initializing this Filter with StringFilter.new(['some/path', 'other/path'])
+    def initialize(filter_argument)
+      filter_argument.map! do |arg|


I think we should rather not use map! as I don't wanna mutate whatever was passed in, so rather without ! :)

PragTob · 2017-06-20T07:46:06Z

lib/simplecov/filter.rb

+        if arg.is_a?(SimpleCov::Filter)
+          arg
+        else
+          Filter.class_for_argument(arg).new(arg)


just thinking out loud, couldn't we also put that check into Filter.class_for_argument e.g. if it is already a filter? Would make that code easier and centralize the knowledge :)

Ah wait we can't because this one already instantiates which we wouldn't need there... maybe another helper method? Not quite sure.

Yah sounds good. I'll make a factory function that instantiates based the appropriate class type, or does nothing if it's given a filter.

PragTob · 2017-06-20T07:47:31Z

lib/simplecov/filter.rb

+      end
+
+      super(filter_argument)
+    end


I have a feeling, the additions here were necessary because THINGS weren't working. Sounds like we might miss some specs here. If you wanna add those, great! 👍

If not let me know and I'll try to get to it when I got some time :)

There is definitely a hole in the specs. I had to do manual tests to make sure this worked. I'll see if I can whip up some specs.

PragTob

Cool stuff! Some question about two usages but otherwise should be ready to roll. Thanks so much! 🚀

PragTob · 2017-06-21T14:48:10Z

lib/simplecov/filter.rb

+    def self.build_filter(filter_argument)
+      return filter_argument if filter_argument.is_a?(SimpleCov::Filter)
+      class_for_argument(filter_argument).new(filter_argument)
+    end


PragTob · 2017-06-21T14:48:23Z

doc/editor-integration.md

+#### [Sublime Editor: SimpleCov](https://packagecontrol.io/packages/SimpleCov)
+*by sentience*
+
+Adds in editor live coverage highlighting, status bar coverage information, and summary coverage information.


👍 drive by fixes

PragTob · 2017-06-21T14:49:25Z

lib/simplecov/filter.rb

    def self.class_for_argument(filter_argument)
+      return filter_argument if filter_argument.is_a?(SimpleCov::Filter)


do we need this? I thought if needed this should be handled by .build_filter ?

Definitely not. Not sure how I put that in there.

PragTob · 2017-06-21T14:49:56Z

lib/simplecov/filter.rb

@@ -66,15 +73,15 @@ def matches?(source_file)

  class ArrayFilter < SimpleCov::Filter
    def initialize(filter_argument)
-      filter_argument.map! do |arg|
+      filter_objects = filter_argument.map do |arg|
        if arg.is_a?(SimpleCov::Filter)
          arg
        else
          Filter.class_for_argument(arg).new(arg)


didn't we wanna use .build_filter here? :)

Right thanks. I knew there was a reason I abstracted that out.

jsteel · 2017-06-22T14:30:07Z

This is a more correct way to get the projects directory, agreed?

jsteel · 2017-06-22T14:30:48Z

The changes in here will also fix #419 I believe

PragTob

I think this looks good and should be good to go in when I have a silent minute :)

PragTob · 2017-06-22T15:22:25Z

lib/simplecov/source_file.rb

@@ -82,8 +82,7 @@ def initialize(filename, coverage)

    # The path to this source file relative to the projects directory
    def project_filename
-      project_dir = File.dirname(File.expand_path(File.basename(__FILE__)))
-      @filename.sub(/^#{project_dir}/, "")
+      @filename.sub(/^#{SimpleCov.root}/, "")


hm not too sure but I think it looks good 👍

In the future though, would be preferred to have unrelated changes for other issues etc. in their own PR so they don't get too big and stay more focussed.

Agreed, but this change was kind of required for the regex. Without it the regex would be matching the full path, which probably nobody would understand.

Ah I see, didn't make that connection in my head. Thanks!

Previously, filter strings passed to `add_filter` have been handled as regexps. It means, for example, `add_filter '.pl'` matches against 'sample.rb' because `.` is handled as _any character_ in regexp. As now we have [regexp filter](simplecov-ruby#589), there's no reason to handle filter strings as regexps. However we need to think about semver with this change. If this behavior was intentional, we cannot introduce this change until next major version bump. Or we can do if this was a _bug_.

@jsteel

0.15.0 (2017-08-14) ([changes](simplecov-ruby/simplecov@v0.14.1...v0.15.0)) ======= ## Enhancements * Ability to use regex filters for removing files from the output. See [#589](simplecov-ruby/simplecov#589) (thanks @jsteel) ## Bugfixes * Fix merging race condition when running tests in parallel and merging them. See [#570](simplecov-ruby/simplecov#570) (thanks @jenseng) * Fix relevant lines for unloaded files - comments, skipped code etc. are correctly classigied as irrelevant. See [#605](simplecov-ruby/simplecov#605) (thanks @odlp) * Allow using simplecov with frozen-string-literals enabled. See [#590](simplecov-ruby/simplecov#590) (thanks @pat) * Make sure Array Filter can use all other filter types. See [#589](simplecov-ruby/simplecov#589) (thanks @jsteel) * Make sure file names use `Simplecov.root` as base avoiding using full absolute project paths. See [#589](simplecov-ruby/simplecov#589) (thanks @jsteel)

Jonathan Steel added 2 commits June 16, 2017 14:35

Allow for regular expression filters

a0b14a1

Fixes simplecov-ruby#514

The parent directories should not match filters

ddc1008

PragTob approved these changes Jun 16, 2017

View reviewed changes

Allow array filters to accept any type

5f88bb5

PragTob reviewed Jun 20, 2017

View reviewed changes

Jonathan Steel added 2 commits June 21, 2017 09:06

Note about sublime integration

e31bce9

Avoid clobbering passed in arguments and minor cleanup

7250a70

PragTob reviewed Jun 21, 2017

View reviewed changes

Jonathan Steel added 2 commits June 21, 2017 11:28

Use the refactored code in all cases

b4ca146

Fix project path to use the SimpleCov.root

cc8b81d

PragTob reviewed Jun 22, 2017

View reviewed changes

PragTob merged commit 44064c3 into simplecov-ruby:master Jun 24, 2017

PragTob mentioned this pull request Jun 24, 2017

Groups use absolute paths #419

Closed

PragTob added a commit that referenced this pull request Jun 24, 2017

Changelog entries for #589 [ci-skip]

92b01f8

yujinakayama mentioned this pull request Dec 4, 2017

Stop handling string filters as regexps #616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regex filters #589

Add regex filters #589

jsteel commented Jun 16, 2017

PragTob left a comment

PragTob Jun 16, 2017

jsteel commented Jun 19, 2017

PragTob left a comment

PragTob Jun 20, 2017

PragTob Jun 20, 2017

jsteel Jun 20, 2017

PragTob Jun 20, 2017

PragTob Jun 20, 2017

jsteel Jun 20, 2017

PragTob Jun 20, 2017

jsteel Jun 20, 2017

PragTob left a comment

PragTob Jun 21, 2017

PragTob Jun 21, 2017

PragTob Jun 21, 2017

jsteel Jun 21, 2017

PragTob Jun 21, 2017

jsteel Jun 21, 2017

jsteel commented Jun 22, 2017

jsteel commented Jun 22, 2017

PragTob left a comment

PragTob Jun 22, 2017

jsteel Jun 22, 2017

PragTob Jun 23, 2017

		def self.class_for_argument(filter_argument)
		return filter_argument if filter_argument.is_a?(SimpleCov::Filter)

Add regex filters #589

Add regex filters #589

Conversation

jsteel commented Jun 16, 2017

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsteel commented Jun 19, 2017

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsteel commented Jun 22, 2017

jsteel commented Jun 22, 2017

PragTob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment