Improve API #140
Improve API #140
Conversation
siphon/catalog.py
Outdated
| from netCDF4 import Dataset as NC4Dataset | ||
| provider = NC4Dataset | ||
| except ImportError: | ||
| raise ValueError('OPENDAP access requires netCDF4-python to be installed.') |
There was a problem hiding this comment.
Shouldn't this be using ModuleNotFoundError?
There was a problem hiding this comment.
I consider it a ValueError for the specified service, since the user asked for one they can't use.
There was a problem hiding this comment.
Not sure I agree on that still. They CAN use the service (i.e. it's available on the server), but they need a module installed locally.
There was a problem hiding this comment.
Alright, you convinced me.
There was a problem hiding this comment.
So....ModuleNotFoundError seems to be Python 3 only. ImportError?
siphon/catalog.py
Outdated
| elif service == 'HTTPServer': | ||
| provider = urlopen | ||
| else: | ||
| raise ValueError(service + ' does not have a built-in handler') |
There was a problem hiding this comment.
Maybe a bit different spelling? not a supported access method in Siphon?
There was a problem hiding this comment.
Definitely better. Have gone with: is not an access method supported by Siphon.
siphon/catalog.py
Outdated
| def __getitem__(self, item): | ||
| """Return an item either by index or name.""" | ||
| try: | ||
| item + '' # Raises if item not a string |
There was a problem hiding this comment.
Why not check the type? It's a bit more look before you leap I suppose.
There was a problem hiding this comment.
That's pretty much it. And I don't like having to figure out what the right type is (though here it's probably basestring).
|
Line length is set to 95 in the configs already. Where am I missing the problem about line length? |
dopplershift
force-pushed
the
improve-api
branch
from
0602949
to
f998fca
Compare
|
Codacy is loudly complaining about the line length. Can it be silenced? Looks like a few minor flake issues are hanging around. |
dopplershift
force-pushed
the
improve-api
branch
2 times, most recently
from
e335420
to
6242905
Compare
|
Found the codacy one, it uses prospector, which is in |
dopplershift
force-pushed
the
improve-api
branch
from
9abe34d
to
4354417
Compare
|
Looks like we may have to go with |
dopplershift
force-pushed
the
improve-api
branch
from
4354417
to
1cb5123
Compare
|
Note: Safe to ignore codacy - does not approve of Ryan's ask-forgiveness string type check. |
|
And QuantifiedCode wants me to document an internal method. |
dopplershift
force-pushed
the
improve-api
branch
2 times, most recently
from
6d5c28a
to
120afd5
Compare
|
Coverage just showed me that I was doing cdmremote wrong...oops. MOAR TESTS! |
dopplershift
force-pushed
the
improve-api
branch
from
120afd5
to
8104f3a
Compare
|
Do you guys generally do reviews before or after the automated tests are passing? |
|
Some of both? In general it's fine to do before, since review is more about catching things the computers don't catch, especially for us who do this a lot. |
|
Not to mention, as backed up as AppVeyor is on our account right now, it's likely I can address any comments now before this PR runs again... |
When getting an item by string, work as before. For all else, treat as an index into values. Works for catalog refs as well.
Parses the keys for datasets and catalog refs and extracts a datetime using a regex. Then filter using nearest time or a time range.
This gives access to the latest dataset. Partially addresses Unidata#137.
Catchlog was dumping way to much output.
dopplershift
force-pushed
the
improve-api
branch
from
8104f3a
to
3ada1e8
Compare
A lot of changes to smooth over the API:
latestattribute to simplify access to latest dataset (Add latest, etc. attributes to Dataset #137)This leaves adding the
formatoption tosubsetto future work. I also punted on best/collection since those aren't easily programmatically determined.