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

Allow PathLike objects #92

Merged
merged 10 commits into from
Jan 14, 2021
Merged

Allow PathLike objects #92

merged 10 commits into from
Jan 14, 2021

Conversation

carloshorn
Copy link
Collaborator

This PR allows pygac to open PathLike objects, that provide an open method.
This is motivated by the new satpy feature from pytroll/satpy#1439 and the subsequent PR pytroll/satpy#1470, where @sfinkens asked to move the implementation to pygac.

@ghost
Copy link

ghost commented Dec 11, 2020 •
edited by ghost
Loading

Congratulations 🎉. DeepCode analyzed your code in 6.66 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@carloshorn
Copy link
Collaborator Author

Hi @sfinkens, @mraspaud,

It appears to me like @stickler-ci does only know python 2 syntax, because it does not know the exception FileNotFoundError.
Reading the deepcode-ci-bot comments, I will refactor the file_opener method, so please wait with the reviews.

Another thing related to this PR, would you mind, if we stop supporting python 2 and update @stickler-ci to check for python 3 syntax?

@mraspaud
Copy link
Member

Yes, please do update .stickler.yaml for python 3. You can check in the satpy repo how it's done:
https://github.com/pytroll/satpy/blob/master/.stickler.yml

@sfinkens
Copy link
Member

I'd be ok with dropping Python 2 support

@carloshorn
Copy link
Collaborator Author

I'd be ok with dropping Python 2 support

Great, I would propose to align with satpy and set the required python version to >=3.6

@mraspaud
Copy link
Member

I'd be ok with dropping Python 2 support

Great, I would propose to align with satpy and set the required python version to >=3.6

Go ahead 👍

@codecov
Copy link

codecov bot commented Dec 14, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@2962a86). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #92   +/-   ##
=========================================
  Coverage          ?   66.30%           
=========================================
  Files             ?       34           
  Lines             ?     2858           
  Branches          ?        0           
=========================================
  Hits              ?     1895           
  Misses            ?      963           
  Partials          ?        0

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 2962a86...166cbbd. Read the comment docs.

This was referenced Dec 14, 2020
Open
Closed
Merged
sfinkens
sfinkens approved these changes Dec 15, 2020
View reviewed changes
Copy link
Member

@sfinkens sfinkens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Just one question and a typo.

pygac/utils.py Outdated Show resolved Hide resolved
pygac/utils.py Outdated Show resolved Hide resolved
pygac/utils.py
def file_opener(file):
if isinstance(file, io.IOBase) and file.seekable():
# avoid closing the file using nullcontext
open_file = nullcontext(file)
Copy link
Member

Choose a reason for hiding this comment

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

Neat!

@carloshorn
Copy link
Collaborator Author

To satisfy codebeat, I guess we will need a huge refactor of the Reader class and its subclasses, but this is beyond the scope of this PR.

mraspaud
mraspaud approved these changes Jan 14, 2021
View reviewed changes
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for implementing this.

@mraspaud mraspaud merged commit c86b67e into pytroll:master Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfinkens sfinkens sfinkens approved these changes

@mraspaud mraspaud mraspaud approved these changes

Assignees

@carloshorn carloshorn

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@carloshorn @mraspaud @sfinkens @stickler-ci