Stop handling string filters as regexps #616
Conversation
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_.
|
👋 Hi, first thanks for your contribution 🎉 This is an interesting issue and a very astute observation! A problem is that for all that I can tell it has been behaving like this forever. However, the README doesn't seem to mention that it has regexp like behaviour: All in all, I'm in favor, the example you provided is especially frightening 😱 So thanks for that! There's another issue I might need to reach out to Christoph for, just wanna double check with him as I'm not aware of all the history/intentions. Regarding version, in
If we fix it I'd internally classify it as a "bug" but as it breaks behaviour old as time I'd still bump a minor to release it so people can have a proper warning looking at the changelog because patch level updates (imo even in 0.y.z) should be safe. |
| it "doesn't match a parent directory with a new SimpleCov::StringFilter" do | ||
| parent_dir_name = File.basename(File.expand_path("..", File.dirname(__FILE__))) | ||
| expect(SimpleCov::StringFilter.new(parent_dir_name)).not_to be_matches subject | ||
| end | ||
|
|
||
| it "matches a new SimpleCov::StringFilter '/fixtures/'" do | ||
| expect(SimpleCov::StringFilter.new("sample.rb")).to be_matches subject | ||
| end |
There was a problem hiding this comment.
why id you remove this spec, shouldn't that still work?
There was a problem hiding this comment.
Because these are exactly same (though the latter description is wrong):
https://github.com/colszowka/simplecov/blob/c5e8654834e922d3e69a980aef029d4bcc880b6e/spec/filters_spec.rb#L25-L27
https://github.com/colszowka/simplecov/blob/c5e8654834e922d3e69a980aef029d4bcc880b6e/spec/filters_spec.rb#L34-L36
Maybe a mistake of copy-pasting?
There was a problem hiding this comment.
Ah yes, missed that! Seems like it was meant to be "/fixtures/" indeed! Unless I missed that one as well :)
I forgot the fact that simplecov is still under 1.0 👍 |
|
Hey all, sorry for my late reply! Thanks for the PR, looks very reasonable to me! As to the potentially breaking change: Personally, I'd say I originally built it like this out of shear laziness. I agree that it should not automatically expand into a regexp under the hood and magically. I also agree that this could be a breaking change for existing users, since a significant behaviour changes. Under SemVer that should warrant a major version bump. We really should finally declare something 1.0 😁 I think what @PragTob suggested, to bump minor version should be fine 🚀 |
|
Well then Thanks @colszowka |
The filter for spec/ was actually matching basically everything in the
gem (and discarding it), because it's all in lib/rspec/, and simplecov
is doing a string-based infix match. I'm not certain when that changed,
but my guess is 2018, in this PR:
simplecov-ruby/simplecov#616
I also adjusted the minimum_coverage down to 99, since we're currently
at 99.26, and not 100%. I'll get on that shortly.
Previously, filter strings passed to
add_filterhave been handled as regexps. It means, for example,add_filter '.pl'matches againstsample.rbbecause.is handled as any character in regexp.As now we have regexp filter, 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.
What do you think?