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 build #129

Merged
merged 7 commits into from
Jul 19, 2017
Merged

Fix build #129

merged 7 commits into from
Jul 19, 2017

Conversation

dopplershift
Copy link
Member

@dopplershift
Copy link
Member Author

Sphinx gallery fails due to Unicode problems. I'm waiting to see the resolution of sphinx-gallery/sphinx-gallery#246 before I decide whether to disable our windows doc builds, and hence example runs.

This was referenced May 31, 2017
0.10.0 supports multiline, but accidentally changed the defaults. Just
go ahead and specify.
@dopplershift dopplershift force-pushed the fix-build branch 2 times, most recently from bc42990 to 53108d3 Compare July 19, 2017 01:26
@dopplershift
Copy link
Member Author

@lesserwhirls @jrleeman One of you want to look this over? It passed before I rebased. Need this to go in before I do my next big PR with our API updates.

@jrleeman
Copy link
Contributor

Having a look now.

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.

Lots of triple quote goodness! A couple of minor wording things that can probably be ignored.

@@ -1,6 +1,7 @@
# Copyright (c) 2013-2015 University Corporation for Atmospheric Research/Unidata.
# Distributed under the terms of the MIT License.
# SPDX-License-Identifier: MIT
"""Tools for accessing atmospheric and oceanic science data on remote servers."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't AOS a bit limited considering lots of folks run THREDDS? But then again this isn't a THREDDS specific library. Maybe something like : Tools for accessing remote data hosted on THREDDS and other servers? Maybe that's too general?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only TDSs I've encountered in the wild are AOS. I'd say it's ok for now, especially since it is kind of the target audience, and siphon is still pretty unknown (I think).

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

@@ -1,6 +1,7 @@
# Copyright (c) 2016 University Corporation for Atmospheric Research/Unidata.
# Distributed under the terms of the MIT License.
# SPDX-License-Identifier: MIT
"""Provide access to TDS' Coverage Dataset."""
Copy link
Contributor

Choose a reason for hiding this comment

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

TDS' took me a couple of glances. Maybe this reads cleaner? Provide access to a THREEDS server's coverage dataset

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it does look odd. Maybe "Provide access to a TDS Coverage Dataset."

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Fixed.

@@ -335,15 +337,14 @@ def make_access_urls(self, catalog_url, all_services, metadata=None):
self.access_urls = access_urls

def add_access_element_info(self, access_element):
"""Create an access method from a catalog element."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it that sometimes we use a regular string

"""blah blah blah"""

but others, we use a raw string (like line 347 below)

r"""blah blah blah"""

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point I was in the habit of using raw to make sure LaTeX math renders properly. Not really needed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nuked 'em.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I may have added some of them as well, no biggie.

@lesserwhirls
Copy link
Collaborator

Should have done this as a review, not single comments...sorry about that.

facepunch

This gets us a clean pass with flake8-docstrings.
Pytest 3.1.0 broke our current use of capfd, but that doesn't seem to be
a good way to handle capturing of log messages. (pytest-dev/pytest#2079)
So instead use pytest-catchlog, which was recommended in that issue and
seems to be maintained.
Need to wait until sphinx-gallery has a release with
sphinx-gallery/sphinx-gallery#246 in it.
Instead of passing the increment and calculated end to numpy.arange(),
just make a range of integers out to the number of points, multiply by
inc, and add start. Also fixes small bug where tolist() was called on
array, but results were not saved anywhere.
@dopplershift
Copy link
Member Author

@jrleeman you may want to look over the last commit.

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 a sane way to handle it.

@dopplershift dopplershift merged commit 4bee767 into Unidata:master Jul 19, 2017
@dopplershift dopplershift deleted the fix-build branch July 19, 2017 22:25
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.

3 participants