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.

Sign up for GitHub

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

Jump to bottom

filter: Use print_err and AugurError #931

Merged
merged 3 commits into from
Jan 17, 2023
Merged

filter: Use print_err and AugurError #931

merged 3 commits into from
Jan 17, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented May 17, 2022 •
edited
Loading

Description of proposed changes

See commit messages.

Related issue(s)

Testing

  • Updated pytests to expect AugurErrors and check error messages.

  • Updated cram tests to reflect return code from:

    augur/augur/__init__.py

    Lines 78 to 80 in 440f7c6

    except AugurError as e:
    print_err(f"ERROR: {e}")
    sys.exit(2)

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.

@victorlin victorlin requested a review from a team May 17, 2022 17:58
@victorlin victorlin self-assigned this May 17, 2022
@victorlin victorlin mentioned this pull request May 17, 2022
Closed
21 tasks
@codecov
Copy link

codecov bot commented May 17, 2022 •
edited
Loading

Codecov Report

Base: 68.10% // Head: 68.07% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (1349a9a) compared to base (0127cc8).
Patch coverage: 88.00% of modified lines in pull request are covered.

❗ Current head 1349a9a differs from pull request most recent head d4eb385. Consider uploading reports for the commit d4eb385 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
- Coverage   68.10%   68.07%   -0.04%     
==========================================
  Files          62       62              
  Lines        6682     6672      -10     
  Branches     1637     1636       -1     
==========================================
- Hits         4551     4542       -9     
+ Misses       1825     1824       -1     
  Partials      306      306
Impacted Files Coverage Δ
augur/filter/include_exclude_rules.py 97.27% <50.00%> (ø)
augur/filter/_run.py 96.35% <80.00%> (-0.04%) ⬇️
augur/filter/validate_arguments.py 88.23% <83.33%> (+1.27%) ⬆️
augur/filter/__init__.py 100.00% <100.00%> (ø)
augur/filter/io.py 76.92% <100.00%> (-0.58%) ⬇️
augur/filter/subsample.py 98.76% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@victorlin victorlin force-pushed the filter/use-common-print-functions branch 4 times, most recently from 84992ba to 3cb4771 Compare May 28, 2022 00:51
@victorlin victorlin force-pushed the filter/use-common-print-functions branch from 3cb4771 to 20b276b Compare July 1, 2022 23:59
@victorlin victorlin force-pushed the filter/use-common-print-functions branch from 20b276b to f657832 Compare July 21, 2022 00:04
@victorlin
Copy link
Member Author

victorlin commented Jul 21, 2022 •
edited
Loading

Rebased onto #997 so 13f46d4 is the only change to review here.

I can't update the GitHub base branch to victorlin:filter/split-file since it's from a fork.

@victorlin victorlin force-pushed the filter/use-common-print-functions branch 2 times, most recently from 6fb437a to 13f46d4 Compare July 21, 2022 00:14
joverlee521
joverlee521 approved these changes Jul 21, 2022
View reviewed changes
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

🌟

@victorlin
Copy link
Member Author

Will rebase/merge this after #997 goes through.

@victorlin
filter.validate_arguments: Use AugurError
243680f 
Simplify the implementation of validate_arguments() to raise AugurErrors
directly instead of returning a boolean to be translated to a proper
error message by the caller.
@victorlin
filter: Use AugurError and print_err
1349a9a 
Use AugurError for consistency in top-level error handling with message
formatting and exit codes.

Use print_err for consistency in outputting warning messages.
@victorlin victorlin force-pushed the filter/use-common-print-functions branch from 13f46d4 to 1349a9a Compare January 17, 2023 21:23
@victorlin victorlin merged commit 993dd81 into nextstrain:master Jan 17, 2023
@victorlin victorlin deleted the filter/use-common-print-functions branch January 17, 2023 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joverlee521 joverlee521 joverlee521 approved these changes

Assignees

@victorlin victorlin

Labels
None yet
Projects
No open projects
Nextstrain planning (archived)
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@victorlin @joverlee521