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

Ensure file handlers all use filenames as strings #455

Merged
merged 1 commit into from Oct 11, 2018

Conversation

honnorat
Copy link
Contributor

@honnorat honnorat commented Oct 9, 2018

All subclasses of BaseFileHandler have been reviewed to be sure that their constructor Subclass.__init__ uses BaseFileHandler.filename instead of the constructor argument.

BaseFileHandler.filename is forced to be a string. The result is that pathlib.Path or strings can be used equally for building an instance of any subclass.

@djhoese
Copy link
Member

djhoese commented Oct 9, 2018

Why didn't you force str(filename) when the base reader class creates the file handlers like I mentioned in #453?

@honnorat
Copy link
Contributor Author

honnorat commented Oct 9, 2018

What do you mean by "the base reader class" ?

@djhoese
Copy link
Member

djhoese commented Oct 9, 2018

Sorry I was trying to catch a bus and had to type fast 😜

I meant FileYAMLReader the "reader" class that creates the individual "file handler" classes. If you changed this line I think it would do the equivalent: https://github.com/pytroll/satpy/blob/master/satpy/readers/yaml_reader.py#L396

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 72.99% when pulling 8cb66eb on EXWEXs:fix-basefilehandler into 38fb064 on pytroll:master.

@codecov
Copy link

codecov bot commented Oct 9, 2018

Codecov Report

Merging #455 into master will increase coverage by 0.01%.
The diff coverage is 56.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   72.97%   72.98%   +0.01%     
==========================================
  Files         134      134              
  Lines       17695    17686       -9     
==========================================
- Hits        12913    12909       -4     
+ Misses       4782     4777       -5
Impacted Files Coverage Δ
satpy/readers/safe_sar_c.py 20.1% <ø> (+0.1%) ⬆️
satpy/readers/native_msg.py 15.02% <ø> (+0.07%) ⬆️
satpy/readers/iasi_l2.py 98.78% <ø> (-0.02%) ⬇️
satpy/readers/hdf4_caliopv3.py 31.57% <ø> (+0.54%) ⬆️
satpy/tests/test_yaml_reader.py 98.64% <ø> (-0.02%) ⬇️
satpy/readers/hdfeos_l1b.py 14.71% <0%> (ø) ⬆️
satpy/readers/li_l2.py 23.88% <0%> (+0.35%) ⬆️
satpy/readers/maia.py 19.27% <0%> (ø) ⬆️
satpy/readers/fci_fdhsi.py 20.22% <0%> (+0.22%) ⬆️
satpy/readers/nc_nwcsaf.py 17.02% <0%> (ø) ⬆️
... and 9 more

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 38fb064...8cb66eb. Read the comment docs.

@honnorat
Copy link
Contributor Author

I see your point. The advantage of this PR is that the changes it proposes are independent of the complex machinery in satpy.readers.yaml_readers. It ensures that when you use the FileHandlers, you are sure that MyFileHandler.filename is a string, whatever the way it was created.

All changes in the subclasses only make this use uniform across the entire code base.

Your solution is good enough for today, though.

@djhoese djhoese merged commit 1bc8349 into pytroll:master Oct 11, 2018
@djhoese djhoese changed the title Ensures BaseFileHandler.filename is a str and is used by its subclasses. Ensure file handlers all use filenames as strings Oct 11, 2018
@djhoese djhoese added enhancement code enhancements, features, improvements component:readers labels Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:readers enhancement code enhancements, features, improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review subclasses of BaseFileHander
3 participants