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

Enforce types to io.open_file #1250

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Enforce types to io.open_file #1250

merged 4 commits into from
Jul 5, 2023

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jul 3, 2023

Description of proposed changes

Previously, the documentation described a type restriction on path_or_buffer, but it was not enforced. This PR updates the type restrictions and enforces them.

Related issue(s)

N/A, motivated by the recent adoption of Mypy type checking.

Testing

  • Added a pytest to show change in behavior.
  • 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.

@victorlin victorlin changed the title Enforce types to open_file Enforce types to io.open_file Jul 3, 2023
@victorlin victorlin self-assigned this Jul 3, 2023
@victorlin victorlin force-pushed the victorlin/update-open_file branch from 0eccb7f to 0a7612e Compare July 3, 2023 18:30
This function is written in a way that supports more than just StringIO.
@victorlin victorlin force-pushed the victorlin/update-open_file branch from 0a7612e to b004bfe Compare July 3, 2023 19:08
@victorlin victorlin marked this pull request as ready for review July 3, 2023 19:30
@victorlin victorlin requested a review from a team July 3, 2023 19:30
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.

Seems reasonable!

@victorlin victorlin force-pushed the victorlin/update-open_file branch from b004bfe to 69145f6 Compare July 5, 2023 20:55
@victorlin victorlin force-pushed the victorlin/update-open_file branch from 69145f6 to 205b70e Compare July 5, 2023 20:56
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (9e8c9e0) 68.96% compared to head (2d02ec2) 68.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1250      +/-   ##
==========================================
+ Coverage   68.96%   68.97%   +0.01%     
==========================================
  Files          64       64              
  Lines        6946     6949       +3     
  Branches     1696     1698       +2     
==========================================
+ Hits         4790     4793       +3     
  Misses       1851     1851              
  Partials      305      305              
Impacted Files Coverage Δ
augur/io/file.py 100.00% <100.00%> (ø)

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

victorlin added 2 commits July 5, 2023 14:50
Previously, anything that couldn't be opened with xopen would be
returned as-is. The docstring described a type restriction on
path_or_buffer but it was not enforced.

Instead of adding a compile-time type restriction on path_or_buffer, add
run-time type restrictions by using type narrowing¹ to provide clear
context to static type checkers and raise a meaningful error if the type
is not supported.

¹ https://mypy.readthedocs.io/en/stable/type_narrowing.html
@victorlin victorlin force-pushed the victorlin/update-open_file branch from 205b70e to 2d02ec2 Compare July 5, 2023 21:51
@victorlin victorlin merged commit edf06e6 into master Jul 5, 2023
@victorlin victorlin deleted the victorlin/update-open_file branch July 5, 2023 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants