Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Convert remote files to FSFile objects automatically #2096
Convert remote files to FSFile objects automatically #2096
Changes from all commits
0ed9b23
36e4b1d
5b189bd
35592f8
6331ff8
b379e5f
810d927
aa876d7
180aba9
ae7b4ca
a7f6b69
59a7caa
f35d010
7c5e41c
f66a22b
bbd50c1
67b28ac
2a8f4ca
b31488d
521c3f6
45ff2aa
69eb6a1
58ae0ce
28de307
e62fd87
f442165
4cd2f4a
7ef8b6f
1c27ea6
d508067
0f2c7d9
c6fc935
361e4d0
fd3156b
00bc379
1fe6dd6
c45fd89
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the doctests skip this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the doctests run by default on all files? I don't see any skips on https://satpy.readthedocs.io/en/stable/quickstart.html which definitely wouldn't work without having the data on the defined path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, ok, I thought we were doctesting everything. I got confused by the doctest setup at the top of the file. Should we have a clear comment on the top of the files that are not tested? Should this file actually be tested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know what that meant so just copied it from
quickstart.rst
where the documentation was originally.I'm not sure these can be tested apart from the GOES case, but I'm also not sure whether having the tests to access AWS/S3 and interact with the data is reasonable...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I want regular unit tests running S3 downloads, but doctests may be "OK". I don't think we run doctests as part of CI as most of our examples use fake non-existent paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a future PR I'll really need to include this information in a big table of all the readers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot: @BENR0 started on that front in #1547 but the PR has been at draft stage since Feb 2021.