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

Fix missing 'rb' mode for opening files #2077

Merged
merged 3 commits into from Apr 7, 2022

Conversation

pdebuyl
Copy link
Contributor

@pdebuyl pdebuyl commented Mar 31, 2022

Add a "mode" argument to the calls to generic_open. This was the case before
using this context manager and was dropped by mistake in 78ef550

This was not caught by tests as the tests are done with mocks instead of
data files.

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Mar 31, 2022

The failing CI error is

 E   ImportError: cannot import name 'Markup' from 'jinja2' (/usr/share/miniconda3/envs/test-environment/lib/python3.9/site-packages/jinja2/__init__.py)

at https://github.com/pytroll/satpy/runs/5766689500?check_suite_focus=true#step:7:219

It appears unrelated to the PR unfortunately.

@mraspaud
Copy link
Member

@pdebuyl Thanks a lot for this! We are aware of the problem in the CI, so don't worry about these kinds of problems.

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.

Thanks for the fix! do you think you can add a test for this, to avoid possible regressions in the future?

@pdebuyl pdebuyl requested a review from djhoese as a code owner March 31, 2022 12:34
@pdebuyl
Copy link
Contributor Author

pdebuyl commented Mar 31, 2022

I add a text test and a binary test, both plain and bzipped.

@mraspaud
Copy link
Member

mraspaud commented Apr 6, 2022

@pdebuyl a fix for the failing ci is now available in the main branch. Merging main into your branch should solve these issues.

@codecov
Copy link

codecov bot commented Apr 6, 2022

Codecov Report

Merging #2077 (fc5f776) into main (48b2356) will increase coverage by 0.00%.
The diff coverage is 93.54%.

@@           Coverage Diff           @@
##             main    #2077   +/-   ##
=======================================
  Coverage   93.85%   93.85%           
=======================================
  Files         283      283           
  Lines       42279    42307   +28     
=======================================
+ Hits        39681    39709   +28     
  Misses       2598     2598           
Flag Coverage Δ
behaviourtests 4.73% <0.00%> (-0.01%) ⬇️
unittests 94.42% <93.54%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
satpy/readers/seviri_l1b_hrit.py 90.14% <0.00%> (ø)
satpy/readers/hrit_base.py 81.87% <100.00%> (ø)
satpy/tests/reader_tests/test_utils.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 48b2356...fc5f776. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 6, 2022

Coverage Status

Coverage increased (+0.004%) to 94.36% when pulling fc5f776 on pdebuyl:fix_generic_open_mode into 48b2356 on pytroll:main.

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Apr 6, 2022

Hi, I merged and force-pushed. The tests pass except for codebeat.

Copy link
Member

@sfinkens sfinkens left a comment

Choose a reason for hiding this comment

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

Thanks @pdebuyl for the fix! The relevant lines are still not covered, but currently I see no way to change this without adding even more mocks...

@sfinkens
Copy link
Member

sfinkens commented Apr 7, 2022

We really need to dynamically write a little test data file and use that for testing to get rid of all the open mocks... I'll put that on my PCW list.

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Apr 7, 2022

Do you think of having actual HRIT files for testing? Else, you would only test the tests... One solution would be to have a "data" repository that could be used optionally when running the tests.

@sfinkens
Copy link
Member

sfinkens commented Apr 7, 2022

I mean writing a small HRIT file during test setup. We already have the data type definitions for the header and image data, so maybe there's a way to write the header and a small image (like 10 scanlines or so) into a valid HRIT file.

@mraspaud
Copy link
Member

mraspaud commented Apr 7, 2022

I like the idea of creating an HRIT file on the fly for testing :)
@pdebuyl we decided a while ago against having data files in the satpy repo. We support a lot of formats, having test data files would make the repo grow too much.

@pdebuyl
Copy link
Contributor Author

pdebuyl commented Apr 7, 2022

I meant another repository. But indeed, generating small HRIT segments is also an option.

Is there anything else regarding the current pull request?

@mraspaud
Copy link
Member

mraspaud commented Apr 7, 2022

I think we're good, maybe @gerritholl wants to give it a spin before we merge?

@mraspaud
Copy link
Member

mraspaud commented Apr 7, 2022

I ended up testing it myself, and it works fine. Merging

@mraspaud mraspaud merged commit 5f5e283 into pytroll:main Apr 7, 2022
@pdebuyl pdebuyl deleted the fix_generic_open_mode branch April 7, 2022 14:42
@pdebuyl
Copy link
Contributor Author

pdebuyl commented Apr 7, 2022

Thanks, and sorry for putting a bug there!

@mraspaud
Copy link
Member

mraspaud commented Apr 7, 2022

No problem, thanks for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating scene with SEVIRI HRIT reader fails with UnicodeDecodeError
5 participants