Spc

[AodhanSweeney]

Adding SPC

[lesserwhirls]

I had no idea Siphon was failing in so many places. It's hard for me to tell for sure, but it seems like it's not been passing for some time now. The good news is that your two classes are not causing any of the errors at this point, which is good.

The next step will be to write some tests for SPC and NCH. To avoid hitting live servers (which can go down and may not be reliable for testing purposes), we utilize vcrpy under the hood. Here is an example:

https://github.com/Unidata/siphon/blob/master/siphon/tests/test_wyoming.py

(the vcrpy library gets pulled in by from siphon.testing import get_recorder).

Basically, the first time you run the test code, any web request gets "recorded" by vcrpy in a "cassette" that will be played back each time you run the test in the future. These "cassettes" are stored in the github repository just like the code, and are found here.

It's not super complicated, but it's a little tricky the first time you go through the testing processing like this.

[zbruick]

I'll get my hands dirty with siphon and work on fixing the Travis errors.

[AodhanSweeney]

Sorry guys just seeing this now but I will make sure to get on the tests. Thanks for the heads up

…

On Thu, Jun 27, 2019 at 4:09 PM Zach Bruick ***@***.***> wrote: I'll get my hands dirty with siphon and work on fixing the Travis errors. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#267?email_source=notifications&email_token=AJUAO4NOQLQEDW5AWGYQLA3P4U3ADA5CNFSM4HZLSKUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYQM4Y#issuecomment-506529395>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJUAO4LKDQG5WDV6ZUFA6HDP4U3ADANCNFSM4HZLSKUA> .

[zbruick]

Note that this PR will close #259, #257, and #228.

[zbruick]

The remaining Travis issue isn't caused by this PR (see discussion on #269). So as far as issues to fix, I think you're good for right now. I think this just needs to go to everyone (@lesserwhirls @dopplershift) for final review now.

[AodhanSweeney]

I saw that it also wanted a copyright so i gave it that as well, I think this is as far as I can take it though... Thanks for all the help though man! @zbruick

[lesserwhirls]

There might be cases where you get another value of 2xx code that is actually ok. Rather than checking for 200 exactly, you can use raise_for_status() on the response, and if something went wrong (4xx, 5xx), requests will raise a requests.exceptions.HTTPError error. See https://3.python-requests.org/user/quickstart/#response-status-codes.

You could replace the whole if block with something like:

try:
    response = requests.get(url)
    response .raise_for_status()
    # rest of code after old if statement
    ...
except requests.exceptions.HTTPError as http_error:
    # give helpful message on console
    print("url {} was not valid, select different storm.".format(url))
    # raise the caught error
    raise    

[lesserwhirls]

Now I'm wondering...since there is only one direct use of the requests library in your PR, perhaps class NHCD should subclass HTTPEndPoint, much like IAStateUpperAir does. @dopplershift @zbruick? Or is that something we refactor later, given the summer timeline?

[zbruick]

Yeah I think consistent methods for accessing data whenever possible across the package would be nice. Since I haven't played around with any of this, I'm not sure how much refactoring it would take, but given how IAStateUpperAir uses it, I don't think it would take that much time. @AodhanSweeney, chime in if this would take too long - not sure what's left on your to-do list for the summer. If too much, we can refactor down the road.

[zbruick]

We would still need to check if the URL was valid though, right? NDBC still does this with _check_if_url_valid, so I don't believe that's handled by HTTPEndPoint?

[lesserwhirls]

I can think of two things to do to address this.

1. replace all r.status_code == 200 checks with r.raise_for_status() (when you have a response obtained from HTTPEndPoint)

2. extend HTTPEndPoint to have a method for checking if a url is reachable (is_url_reachable(url)?) which simply does a head request on the url followed by a raise_for_status() (would replace all _check_if_url_valid type things). Maybe something like:

def is_url_reachable(self, path, params=None):
        """Test if a url is reachable.

        Make a HEAD request, optionally including a parameters, to a path.
        The path of the request is the full URL.
        Parameters
        ----------
        path : str
            The URL to request
        params : DataQuery, optional
            The query to pass when making the request
        Returns
        -------
        resp : requests.Response
            The server's response to the request
        Raises
        ------
        HTTPError
            If the server returns a 4xx or 5xx code

        """
        resp = self._session.head(path, params=params)
        resp.raise_for_status()

        return true

Since get requests follow redirect by default, we should probably make sure the head call includes the parameter allow_redirects=True (head does not follow redirects by default), since the url would ultimately be reachable by a get request.

How does that sound?

[lesserwhirls]

SpcData should also use HTTPEndPoint, in the way that NDBC does. In this case, we would use the methods from HTTPEndPoint to read the data from a URL, and pass that to pandas pd.read_csv by wrapping the HTTPEndPoint return in a StringIO. Again, @dopplershift @zbruick, is that something we refactor later, given the summer timeline?

[AodhanSweeney]

This most recent pull request is good for approval, but I want to work something else out so no one needs to look at it quite yet

[codecov]

Codecov Report

Merging #267 into master will increase coverage by 0.05%.
The diff coverage is 98.5%.

@@            Coverage Diff             @@
##           master     #267      +/-   ##
==========================================
+ Coverage   97.79%   97.85%   +0.05%     
==========================================
  Files          40       44       +4     
  Lines        4318     4716     +398     
==========================================
+ Hits         4223     4615     +392     
- Misses         95      101       +6

Impacted Files	Coverage Δ
siphon/tests/test_nhc.py	100% <100%> (ø)
siphon/tests/test_spc.py	100% <100%> (ø)
siphon/simplewebservice/iastate.py	98.43% <100%> (ø)	⬆️
siphon/simplewebservice/nhc.py	100% <100%> (ø)
siphon/http_util.py	98.18% <100%> (+0.06%)	⬆️
siphon/simplewebservice/spc.py	94.33% <94.33%> (ø)

---

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 881c77d...89ceea3. Read the comment docs.

[AodhanSweeney]

This is now ready for review, once this is in I will be pushing respective GUIs into the python library.

[AodhanSweeney]

This should be ready for review, it addresses issues #228 #259 and #257

[zbruick]

I approve but I'll defer to others on this before merging, as this is pretty extensive.

[lesserwhirls]

The build isn't passing at the moment, but outside of that, what remains on this one?

[dopplershift]

A review of the API for the NHC stuff (signature of public functions) to make sure we're ok with supporting that interface long term.