Fix issue when calling intervals on a CellMethod

[kinow]

Was having a look around the code repositories I have cloned locally, and found at least one interesting issue.

from cf.cellmethod import CellMethod

c = CellMethod.create('lat: mean (interval: 1 hour)')[0]
print(c.intervals)

The property getter was calling the method, but not returning it.

Cheers
Bruno

[sadielbartholomew]

Hi @kinow, thanks for raising this! I'll have a proper look at this shortly but from reading your description it seems like it should do as you say, & the test looks sensible. And before merging I'll double check with David,

BTW, last time you checked in here I remember you requested some CI to help you check on changes. Unfortunately you've just missed it with this PR since I've recently started to create the workflow with GitHub Actions to run the test suite & it is nearly there so I can merge it into master by the end of the week. So it'll be there for next time 😀 Sorry it wasn't ready here, we were focusing on other things & we don't get many PRs from others coming in ATM.

[kinow]

Hi @sadielbartholomew ! We just recently moved some repositories to GitHub actions and we are quite happy.

If we take a bit to review this one, I'd be happy to rebase and see if it would trigger the CI (I never tried that; a PR that didn't have CI, being rebased after it was activated in the master branch... it should work right?)

Thanks!

[sadielbartholomew]

Great spot & sensible fix with a test that fails without it & passes with it. Perfect.

A few one-line changes would be good e.g. to avoid a deprecated test method, & while you are making that I have noticed in testing that there is a comma missing breaking one of the docstring examples that get processed into the docs for the method in question:

cf-python/cf/cellmethod.py

Line 376 in 2ea8bad

>>> c.intervals = [cf.Data(7.5 'minutes')]

(should be 7.5, 'minutes') so if you're happy to, to save me committing just for that one-liner, it would be good if you could pop that in (though no worries if not, I can sort it).

[sadielbartholomew]

We just recently moved some repositories to GitHub actions and we are quite happy.

Good to hear. I like the concept (power, flexibility etc.) & configuration of the Actions but I must admit it's been a bit of a headache getting them setup, mainly because it is not clear or documented well on how it triggers based on which branches workflows are defined in relative to the ones they trigger from events on. But I'm sure I'll become more familiar with that over time.

If we take a bit to review this one, I'd be happy to rebase and see if it would trigger the CI (I never tried that; a PR that didn't have CI, being rebased after it was activated in the master branch... it should work right?)

It shouldn't take that long to get this reviewed & merged. I think it depends on the workflow: I've currently specified [opened, reopened, ready_for_review, edited] as events to trigger off for PRs, & I think a rebase counts as an edit. So when the workflow is in that would work. Shouldn't be necessary though.

[kinow]

Argh, I applied both changes @sadielbartholomew , but the second one my finger was too-quick. I think the function returns an array with all the intervals. So I unpacked them with cm, to get the first element in the list.

Not very clear, so let me fix that and drop/edit that commit. Sorry.

[kinow]

Argh, I applied both changes @sadielbartholomew , but the second one my finger was too-quick. I think the function returns an array with all the intervals. So I unpacked them with cm, to get the first element in the list.

Not very clear, so let me fix that and drop/edit that commit. Sorry.

Done

[kinow]

(should be 7.5, 'minutes') so if you're happy to, to save me committing just for that one-liner, it would be good if you could pop that in (though no worries if not, I can sort it).

Done @sadielbartholomew ! I noticed PyCharm complained about several docstrings. Maybe CI could run these tests too?

[sadielbartholomew]

Thanks. All good.

I noticed PyCharm complained about several docstrings. Maybe CI could run these tests too?

Yes, incorporating automatic checking of the docstrings has been on my TODO list for a while. We'll add it in eventually but it't not high on the priority list. Feel free to submit PRs with fixes or even to add in the docstring validator yourself if you are super keen!

[kinow]

Feel free to submit PRs with fixes or even to add in the docstring validator yourself if you are super keen!

Was already doing a PoC before calling it a day. Will send the draft PR that may very well be postponed as the docstrings would need fixing so that the build passes.

Thanks @sadielbartholomew !

[sadielbartholomew]

Minor feedback addressed. I've just spoken with David & he's is happy with this. Thanks.