-
Notifications
You must be signed in to change notification settings - Fork 36
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
Remove --doc-filter option from CLI. #795
Conversation
31a14c1
to
bda219a
Compare
Codecov Report
@@ Coverage Diff @@
## next #795 +/- ##
==========================================
- Coverage 86.32% 84.99% -1.33%
==========================================
Files 51 54 +3
Lines 4687 4672 -15
Branches 1022 1011 -11
==========================================
- Hits 4046 3971 -75
- Misses 456 522 +66
+ Partials 185 179 -6
Help us with your feedback. Take ten seconds to tell us how you rate us. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding tests for these changes? I am not sure if we already have them or not. Also, I see a lot of lines not covered by us. Although not in scope for this PR, should we worry about this?
The changes looks good to me, if we decide to add tests, happy to re-review :)
@kidrahahjo Thanks for the questions! The new unified filter syntax is already testing document filters, just below where I deleted the tests for the old Codecov is extra noisy on this PR but I don’t think we are really missing anything new. The baseline coverage measurement for the next branch may be wrong because I just force-pushed some changes. I think this PR’s coverage is fine because we’re just removing a feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we don't have to worry about the Codecov diff for now. Otherwise these changes LGTM.
@@ -32,7 +32,7 @@ | |||
from . import get_project, init_project | |||
from .common import config | |||
from .common.configobj import Section, flatten_errors | |||
from .contrib.filterparse import _add_prefix, parse_filter_arg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that _add_prefix
can be removed entirely at this point. I can verify that and create a separate issue for that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Remove --doc-filter from CLI. * Minor docstring improvements. * Remove --doc-filter tests. * Update changelog.
* Remove --doc-filter from CLI. * Minor docstring improvements. * Remove --doc-filter tests. * Update changelog.
* Remove --doc-filter from CLI. * Minor docstring improvements. * Remove --doc-filter tests. * Update changelog.
Description
This PR resolves #613 by removing the
--doc-filter
argument from the signac CLI. Users who wish to search by a document filter should replace syntax as shown below:Motivation and Context
The general feature of searching by document filters has been replaced by a generalized filter syntax that prefixes state point keys with
sp.
and document keys withdoc.
. This applies that change from #586 to the CLI.Checklist: