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

filter: Error when any group-by column is not found #933

Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented May 17, 2022

Description of proposed changes

Actualizing the TODO removed in this change:

TODO: We should consider treating this case as an actual error and exiting here with a nonzero code.

With 2342f07, raising FilterException(AugurError) alone is sufficient to exit with a nonzero code.

Also updated error text to remove message "No sequences-per-group sampling will be done", which was part of a warning. Implicitly, nothing will be done when exiting with an ERROR.

Related issue(s)

Testing

Added cram tests.

@victorlin victorlin requested a review from a team May 17, 2022 22:04
@victorlin victorlin self-assigned this May 17, 2022
@victorlin victorlin force-pushed the filter/error-on-group-by-not-found branch from eef7767 to c8489dc Compare May 17, 2022 22:09
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #933 (c8489dc) into master (ea89ece) will increase coverage by 0.03%.
The diff coverage is 11.11%.

❗ Current head c8489dc differs from pull request most recent head 7cac045. Consider uploading reports for the commit 7cac045 to get more accurate results

@@            Coverage Diff             @@
##           master     #933      +/-   ##
==========================================
+ Coverage   34.58%   34.61%   +0.03%     
==========================================
  Files          42       42              
  Lines        6055     6040      -15     
  Branches     1551     1549       -2     
==========================================
- Hits         2094     2091       -3     
+ Misses       3887     3875      -12     
  Partials       74       74              
Impacted Files Coverage Δ
augur/filter.py 69.06% <11.11%> (+0.50%) ⬆️
augur/refine.py 5.37% <0.00%> (-0.51%) ⬇️
augur/parse.py 62.96% <0.00%> (-0.46%) ⬇️
augur/__init__.py 30.76% <0.00%> (+1.73%) ⬆️
augur/utils.py 44.31% <0.00%> (+4.58%) ⬆️
augur/dates.py 100.00% <0.00%> (+17.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea89ece...7cac045. Read the comment docs.

@victorlin victorlin added the breaking Makes a backwards incompatible change and should wait for major release label May 17, 2022
victorlin added 2 commits May 24, 2022 10:01
Actualizing the TODO removed in this change:

> TODO: We should consider treating this case as an actual error and exiting here with a nonzero code.

With 2342f07, raising FilterException(AugurError) alone is sufficient to exit with a nonzero code.

Also updated error text to remove message "No sequences-per-group sampling will be done", which was part of a warning. Implicitly, nothing will be done when exiting with an ERROR.
@victorlin victorlin force-pushed the filter/error-on-group-by-not-found branch from c8489dc to 7cac045 Compare May 24, 2022 17:01
Copy link
Contributor

@huddlej huddlej left a comment

Choose a reason for hiding this comment

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

Awesome! I love a PR that removes code to fix a bug and adds tests.

@victorlin victorlin merged commit 62cce8b into nextstrain:master May 24, 2022
@victorlin victorlin deleted the filter/error-on-group-by-not-found branch May 24, 2022 17:14
@victorlin victorlin added this to the Major release 16.0.0 milestone Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Makes a backwards incompatible change and should wait for major release
Projects
No open projects
2 participants