Feature/smhi class to match 0.2.0

[docNord]

Naive update of parent class to new structure. To be updated later. Ambition here is simply to support 0.2.0 release.

• [x] Restructure base code to new format
• [x] Fix superstructure (find station from gps and place name, interpolation etc)
• [x] Test functionality
• [x] Re-write tests
• [x] Update documentation

[docNord]

I am not in a position to fix the unit tests without support.

[mgcth]

I hate mutating an object's state. I'd rather a method called get_ returns something and not also mutated an object's internal state.

I'd also write the unit tests in a way that will be easier for us when updating the code in the future. That is, as little patching as possible and assert calls and stuff. The important underlying stuff has been tested and we can assume it'll run fine. I know this isn't strictly a unit test, but they are too brittle to upkeep for our small organisation. Define some input data at some defined interface and some output data your module should produce, mock requests.get, the Metobs modules or so, and let the code run. Call that the unit test of the module. In the integration test, we can actually call the API and run through.

I've simplified it a bit in 57c3e05. I'd probably add a separate method called get_data_by_city (and also allow for interpolation) or so and refactor some of the code in get_data so it can be reused.

[mgcth]

I still think you should refactor the _interpolate method in c4dfcca.

[docNord]

Sorry @mgcth, didn't realize until now that you had also commited here. I will look into your work and update. Agree we shouldn't mutate the object.

[docNord]

I will verify the functionality of the support functions you have refactored out of their respective contexts tomorrow @mgcth and also check that it the functionality is there in the main functions to call for the function as necessary. Some work still to be done.

[docNord]

All the code seems to work now and the adjustments were minor. Will continue to learn about patching schemes this weekend and fix the rest of the tests.

[docNord]

@mgcth now the integration tests for MetObs fail?!?!

[mgcth]

They are testing the live API, which can change at any point. Perhaps you can have a look at

https://github.com/Ingenjorsarbete-For-Klimatet/ifk-smhi/blob/1f2b9889c56568ee1c63d1d9e048f574c0fd7a39/src/smhi/metobs.py#L365

[docNord]

They are testing the live API, which can change at any point. Perhaps you can have a look at

https://github.com/Ingenjorsarbete-For-Klimatet/ifk-smhi/blob/1f2b9889c56568ee1c63d1d9e048f574c0fd7a39/src/smhi/metobs.py#L365

Sure - but shouldn't that be a new PR/issue?

[mgcth]

They are testing the live API, which can change at any point. Perhaps you can have a look at https://github.com/Ingenjorsarbete-For-Klimatet/ifk-smhi/blob/1f2b9889c56568ee1c63d1d9e048f574c0fd7a39/src/smhi/metobs.py#L365

Sure - but shouldn't that be a new PR/issue?

Sure.

[docNord]

There is no "corrected-archive" returned. That is what the code gets hung up about, I think:

from smhi.metobs import Parameters, Stations, Periods, Data 
>>> parameters = Parameters()
>>> stations = Stations(parameters, 21)
>>> periods = Periods(stations, stations.data[1][0])
>>> periods.data
('latest-day', 'latest-hour', 'latest-months')
>>> data = Data(periods)

Returns the following error message:

Traceback (most recent call last):
  File "C:\SCRATCH\ifk-smhi\src\smhi\metobs.py", line 87, in _get_url
    requested_data = [x for x in data if getattr(x, key) == str(parameter)][0]
IndexError: list index out of range

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\SCRATCH\ifk-smhi\src\smhi\metobs.py", line 345, in __init__
    url, _ = self._get_url(periods_in_station.period, "key", period, data_type)
  File "C:\SCRATCH\ifk-smhi\src\smhi\metobs.py", line 92, in _get_url
    raise IndexError("Can't find data for parameters: {p}".format(p=parameter))
IndexError: Can't find data for parameters: corrected-archive

[docNord]

I would argue that the error stems from the initiation of the Data class @mgcth 👍

    def __init__(
        self,
        periods_in_station: Periods,
        period: str = "corrected-archive",
        data_type: str = "json",

Here, period is explicitly set as "corrected-archive". I have a proposal to instead choose the first available period periods_in_station.data[0] which I think is what you have tried to implement a bit further down. I'll implement it and commit it in this PR.

[mgcth]

I would argue that the error stems from the initiation of the Data class @mgcth 👍

    def __init__(
        self,
        periods_in_station: Periods,
        period: str = "corrected-archive",
        data_type: str = "json",

Here, period is explicitly set as "corrected-archive". I have a proposal to instead choose the first available period periods_in_station.data[0] which I think is what you have tried to implement a bit further down. I'll implement it and commit it in this PR.

That wasn't what I tried to implement. It was a special case for when only one period was present. It can probably be generalised to select the period closest to corrected-archive with a user warning if corrected-archive is the default.

[docNord]

...aaand that caused typing errors.

[docNord]

Alright, now we are always sorting alphabetically: ['corrected-archive', 'latest-day', 'latest-hour', 'latest-month'] Will push one more commit to sort.

[docNord]

Alright you will have to approve this now @mgcth . Now integration tests for Strang are failing too, but only for MacOs. Please let me out of this PR!

[mgcth]

Alright, now we are always sorting alphabetically: ['corrected-archive', 'latest-day', 'latest-hour', 'latest-month'] Will push one more commit to sort.

We were always sorting in that order but the order isn't very logical. If we will just select the first item from that list it is hard to predict for the user what they get back. Sorting by hour, day, month and archive or the reverse has some logic to it at least.

[docNord]

Alright, now we are always sorting alphabetically: ['corrected-archive', 'latest-day', 'latest-hour', 'latest-month'] Will push one more commit to sort.

We were always sorting in that order but the order isn't very logical. If we will just select the first item from that list it is hard to predict for the user what they get back. Sorting by hour, day, month and archive or the reverse has some logic to it at least.

Ok, so if you've not said anything you'll get the latest reading? Yeah, that makes sense. I will sort that out. Out of curiosity, did you find out what happened with the MacOs integration test?

[mgcth]

Alright, now we are always sorting alphabetically: ['corrected-archive', 'latest-day', 'latest-hour', 'latest-month'] Will push one more commit to sort.

We were always sorting in that order but the order isn't very logical. If we will just select the first item from that list it is hard to predict for the user what they get back. Sorting by hour, day, month and archive or the reverse has some logic to it at least.

Ok, so if you've not said anything you'll get the latest reading? Yeah, that makes sense. I will sort that out. Out of curiosity, did you find out what happened with the MacOs integration test?

I couldn’t open the link strang errored on. If it works now I think it was an error/maintenance on SMHI’s side.

[docNord]

This is a really ugly and inconvenient abuse of code to circumvent the typing issues @mgcth. Should really be fixed, but I am not able to come up with a more pretty solution right now I'm afraid and at least this works.

[docNord]

Just when I thought we were out of the woods, metobs_integration fails again... 🙄

[docNord]

Got it! ok now @mgcth ?

[docNord]

@mgcth ...?

[docNord]

Thanks for the input. I will try to get an integration test going and sort out the remaining stuff to the extent that I can.

[docNord]

@mgcth it seems your fix to use IntEnum changed the order of the list so that default is now "corrected-archive" again, was that deliberate?

[mgcth]

@mgcth it seems your fix to use IntEnum changed the order of the list so that default is now "corrected-archive" again, was that deliberate?

Yes. Do you prefer it the other way around?

[docNord]

@mgcth it seems your fix to use IntEnum changed the order of the list so that default is now "corrected-archive" again, was that deliberate?

@mgcth it seems your fix to use IntEnum changed the order of the list so that default is now "corrected-archive" again, was that deliberate?

Yes. Do you prefer it the other way around?

Not at all - I thought you wanted it the other way around after that discussion we had earlier, but I have absolutely no opinion on the matter. I will continue with this way of working, seems good to me. 👍

[docNord]

Ping @mgcth

[docNord]

I don't know if I managed to get the figure in the build properly @mgcth, could you have a look please? Other than that, I am quite happy.