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

Report helpful error if filter query string malformed #1232

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

corneliusroemer
Copy link
Member

@corneliusroemer corneliusroemer commented May 18, 2023

Resolves #1230
filter: Improperly handled ValueError: not enough values to unpack (expected 2, got 1) in _parse_filter_query

Description of proposed changes

Validate, and raise error with helpful warning if the query string is malformed, rather than raising unhelpful Value Error

New error output:

❌1 ❯ augur filter --exclude-where "coverage<0.95"   --sequences data/sequences.fasta.zst   --metadata results/metadata.tsv   --exclude config/exclude_accessions_mpxv.txt  --output-sequences results/hmpxv1/good_sequences.fasta.zst  --output-metadata results/hmpxv1/good_metadata.tsv   --min-length 180000   
Note: You did not provide a sequence index, so Augur will generate one. You can generate your own index ahead of time with `augur index` and pass it with `augur filter --sequence-index`.
ERROR: Invalid query `coverage<0.95`. Hint: Queries must follow the pattern of `property=value` or `property!=value`.

Testing

What steps should be taken to test the changes you've proposed?
If you added or changed behavior in the codebase, did you update the tests, or do you need help with this?

  • Checks pass

Checklist

  • Add a message in CHANGES.md summarizing the changes in this PR that are end user focused. Keep headers and formatting consistent with the rest of the file.

Resolves #1230
filter: Improperly handled `ValueError: not enough values to unpack (expected 2, got 1)` in ` _parse_filter_query`
@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.03 ⚠️

Comparison is base (5c1fc41) 68.87% compared to head (c095404) 68.85%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
- Coverage   68.87%   68.85%   -0.03%     
==========================================
  Files          64       64              
  Lines        6937     6938       +1     
  Branches     1693     1693              
==========================================
- Hits         4778     4777       -1     
- Misses       1854     1855       +1     
- Partials      305      306       +1     
Impacted Files Coverage Δ
augur/filter/include_exclude_rules.py 96.61% <60.00%> (-1.12%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@huddlej
Copy link
Contributor

huddlej commented May 31, 2023

@corneliusroemer The exclude-where/include-where syntax was never designed to be a full-featured query syntax, but you could use the --query interface which supports pandas-style query syntax to select the records you want. Instead of "excluding where" coverage<0.95, you would rephrase the query as a filter like --query "coverage >= 0.95".

I agree though that the error message could be improved! As part of the hint here, you might recommend the query flag as an alternative approach...

op = operator.ne
split_query = re.split(r'!?=', query)
if len(split_query) != 2:
raise AugurError(f"Invalid filter query `{query}`. Hint: Queries must follow the pattern of `property=value` or `property!=value`.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for @huddlej's suggestion. Example:

Suggested change
raise AugurError(f"Invalid filter query `{query}`. Hint: Queries must follow the pattern of `property=value` or `property!=value`.")
raise AugurError(f"Invalid filter query `{query}`. Hint: Queries must follow the pattern of `property=value` or `property!=value`. `--query` supports more complex queries.")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

filter: Improperly handled ValueError: not enough values to unpack (expected 2, got 1) in _parse_filter_query
3 participants