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.

Sign up for GitHub

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

Jump to bottom

Improve API #140

Merged
merged 7 commits into from
Jul 22, 2017
Merged

Improve API #140

merged 7 commits into from
Jul 22, 2017

Conversation

dopplershift
Copy link
Member

A lot of changes to smooth over the API:

This leaves adding the format option to subset to future work. I also punted on best/collection since those aren't easily programmatically determined.

@lesserwhirls lesserwhirls self-requested a review July 20, 2017 22:32
@dopplershift dopplershift requested a review from jrleeman July 20, 2017 22:42
jrleeman
jrleeman suggested changes Jul 21, 2017
View reviewed changes
Copy link
Contributor

@jrleeman jrleeman left a comment

Choose a reason for hiding this comment

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

  • Looks like the line length needs to be bumped in flake checks.

  • siphon.ncss.NCSS import unused in examples

siphon/catalog.py Outdated
from netCDF4 import Dataset as NC4Dataset
provider = NC4Dataset
except ImportError:
raise ValueError('OPENDAP access requires netCDF4-python to be installed.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be using ModuleNotFoundError?

Copy link
Member Author

Choose a reason for hiding this comment

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

I consider it a ValueError for the specified service, since the user asked for one they can't use.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, you convinced me.

Copy link
Member Author

Choose a reason for hiding this comment

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

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')
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a bit different spelling? not a supported access method in Siphon?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not check the type? It's a bit more look before you leap I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough

@dopplershift
Copy link
Member Author

Line length is set to 95 in the configs already. Where am I missing the problem about line length?

@jrleeman
Copy link
Contributor

Codacy is loudly complaining about the line length. Can it be silenced?

Looks like a few minor flake issues are hanging around.

@dopplershift dopplershift force-pushed the improve-api branch 2 times, most recently from e335420 to 6242905 Compare July 21, 2017 16:07
@dopplershift
Copy link
Member Author

Found the codacy one, it uses prospector, which is in .prospector.yml.

@jrleeman
Copy link
Contributor

Looks like we may have to go with ImportError... Thanks 2.7!

@jrleeman
Copy link
Contributor

Note: Safe to ignore codacy - does not approve of Ryan's ask-forgiveness string type check.

@dopplershift
Copy link
Member Author

And QuantifiedCode wants me to document an internal method.

@dopplershift dopplershift force-pushed the improve-api branch 2 times, most recently from 6d5c28a to 120afd5 Compare July 21, 2017 19:35
@dopplershift
Copy link
Member Author

Coverage just showed me that I was doing cdmremote wrong...oops. MOAR TESTS!

@dopplershift dopplershift mentioned this pull request Jul 21, 2017
Merged
@dopplershift dopplershift mentioned this pull request Jul 21, 2017
Merged
@lesserwhirls
Copy link
Collaborator

Do you guys generally do reviews before or after the automated tests are passing?

@dopplershift
Copy link
Member Author

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.

@dopplershift
Copy link
Member Author

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...

@dopplershift
ENH: Add ability to access datasets by index (Fixes Unidata#134)
285bcce 
When getting an item by string, work as before. For all else, treat as
an index into values. Works for catalog refs as well.
@dopplershift
ENH: Enable extracting datasets using datetimes (Fixes Unidata#135)
ee47ff8 
Parses the keys for datasets and catalog refs and extracts a datetime
using a regex. Then filter using nearest time or a time range.
@dopplershift
ENH: Adding simpler access methods (Fixes Unidata#136)
01fedf9
@dopplershift
ENH: Add latest attribute to catalog
f11cb78 
This gives access to the latest dataset. Partially addresses Unidata#137.
@dopplershift
MNT: Update examples for API changes
77eeea3
@dopplershift
MNT: Silence the log message on test failure
a65193b 
Catchlog was dumping way to much output.
@dopplershift dopplershift merged commit 7dd6c7a into Unidata:master Jul 22, 2017
@dopplershift dopplershift deleted the improve-api branch July 22, 2017 15:57
@dopplershift dopplershift added this to the 0.5 milestone Jul 22, 2017
@dopplershift dopplershift mentioned this pull request Jul 22, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrleeman jrleeman jrleeman approved these changes

@lesserwhirls lesserwhirls Awaiting requested review from lesserwhirls lesserwhirls is a code owner

Assignees
No one assigned
Labels
Area: Catalog Area: NCSS Type: Enhancement
Projects
None yet
Milestone
0.5
Development

Successfully merging this pull request may close these issues.
3 participants
@dopplershift @jrleeman @lesserwhirls