-
Notifications
You must be signed in to change notification settings - Fork 367
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
Add documentation, kwargs for Reader #2236
Conversation
lib/cartopy/io/shapereader.py
Outdated
Reader = FionaReader | ||
else: | ||
Reader = BasicReader | ||
def Reader(filename, bbox=None, **kwargs): |
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.
Is there a reason to make this change as well? Now Reader
is a function rather than a class.
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 Reader being an alias for BasicReader / FionaReader is ideal. Using a function avoids this and also adds the docstring.
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 Reader being an alias for BasicReader / FionaReader is ideal.
I agree it isn't ideal. But, I personally would prefer that to a function that looks like a class, I won't block on it, but I also won't approve it either.
Another way of doing this would have been to create a class Reader
that stores an instance of self._reader = FionaReader() if _HAS_FIONA else BasicReader()
and then dispatches out the methods to whatever self._reader
it contains, but that seems like more work than is necessary as well unless we start getting more readers.
Also, I think you can add a docstring right below the Reader
definition like we do for the configuration dictionary:
cartopy/lib/cartopy/__init__.py
Lines 23 to 31 in 96ee0a3
config = {'pre_existing_data_dir': Path(os.environ.get('CARTOPY_DATA_DIR', | |
'')), | |
'data_dir': _data_dir, | |
'cache_dir': _cache_dir, | |
'repo_data_dir': Path(__file__).parent / 'data', | |
'downloaders': {}, | |
} | |
""" | |
The config dictionary stores global configuration values for cartopy. |
but maybe to get that to work you'd need to put it all on one line?
Reader = FionaReader if _HAS_FIONA else BasicReader
"""Explanation of Reader"""
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.
Thanks for the tip on adding a docstring to the existing Reader line! I like that solution, which has now been made.
add bbox param back to BasicReader Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
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 like this approach! The API docs render nicely too.
Thanks also @smartlixx and @wqshen for helping spur this along! |
Rationale
There was some discussion on how to best implement support for the encoding keyword for the BasicReader and FionaReader interfaces in #2231, and passing it through kwargs was suggested. There is also a generally lack of documentation on 'Reader' that automatically selects between the two (currently the docs just say "alias of FionaReader").
Implications
This PR does three things:
Add a docstring to shapereader.Reader to document usage
Passes kwargs upstream to fiona and PyShp (notably including 'encoding')
bbox is also now passed to PyShp, since support was added in v2.2.0