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

Move AugurError to new errors.py, replace RuntimeError #921

Merged
merged 2 commits into from
May 27, 2022

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented May 12, 2022

Description of proposed changes

See commit messages.

Related issue(s)

Suggested by @tsibley: #918 (review)

Testing

Tests modified accordingly.

@victorlin victorlin requested a review from a team May 12, 2022 20:34
@victorlin victorlin self-assigned this May 12, 2022
@victorlin
Copy link
Member Author

Cyclic dependency between augur.utils and augur.util_support...

@victorlin victorlin marked this pull request as draft May 12, 2022 21:21
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #921 (2ad9b2b) into master (5916362) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #921      +/-   ##
==========================================
+ Coverage   59.34%   59.36%   +0.02%     
==========================================
  Files          42       43       +1     
  Lines        6011     6014       +3     
  Branches     1539     1539              
==========================================
+ Hits         3567     3570       +3     
  Misses       2185     2185              
  Partials      259      259              
Impacted Files Coverage Δ
augur/utils.py 64.34% <ø> (-0.31%) ⬇️
augur/__init__.py 79.36% <100.00%> (+0.33%) ⬆️
augur/errors.py 100.00% <100.00%> (ø)
augur/filter.py 96.24% <100.00%> (+<0.01%) ⬆️
augur/parse.py 89.02% <100.00%> (ø)
augur/util_support/node_data_file.py 100.00% <100.00%> (ø)

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 5916362...2ad9b2b. Read the comment docs.

@victorlin victorlin marked this pull request as ready for review May 12, 2022 22:11
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.

Thanks, @victorlin! What if we placed AugurError in a new errors.py or exceptions.py top-level module in augur (sort of like pandas does, only simpler)? This organization should address this and future circular import issues. I also prefer from .errors import AugurError over from .utils import AugurError for readability.

@victorlin
Copy link
Member Author

@huddlej good suggestion, done: 2af1045

@victorlin
Copy link
Member Author

Would you rather keep or drop 7a72c25? I'm fine with either.

@victorlin victorlin requested a review from huddlej May 24, 2022 16:36
@victorlin victorlin force-pushed the victorlin/use-augurerror branch from 2af1045 to b4edf53 Compare May 24, 2022 16:48
@huddlej
Copy link
Contributor

huddlej commented May 24, 2022

@victorlin I'd probably drop that change for now. Seeing all of those imports at the top should be a good reminder for us to move the util support logic back into utils at some point... :)

@victorlin victorlin force-pushed the victorlin/use-augurerror branch 2 times, most recently from 3ddbd46 to 6ed70a5 Compare May 24, 2022 16:58
@victorlin victorlin changed the title Replace RuntimeError with AugurError Move AugurError to new errors.py, replace RuntimeError May 24, 2022
victorlin added 2 commits May 24, 2022 14:48
Previously, f04f357 replaced AugurException with RuntimeError.

With 2342f07, AugurException is renamed to AugurError and properly handled in the top-level exception handler.
This reduces complexity and chance of circular imports in utils.
@victorlin victorlin force-pushed the victorlin/use-augurerror branch from 6ed70a5 to 2ad9b2b Compare May 24, 2022 21:51
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

LGTM by inspection. Using a separate "errors" module is also what I'd done for nextstrain.cli.errors, as exception classes are used/imported so widely it's good not to have them wrapped up with other stuff.

@victorlin victorlin merged commit e204d3e into master May 27, 2022
@victorlin victorlin deleted the victorlin/use-augurerror branch May 27, 2022 01:10
@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
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants