grib1 unit 10 handling

[bblay]

replaces #168, now pulling to master

[pp-mo]

👍

But.. do you know if there may be various other similar issues, and are we due a review?

[bblay]

There are bound to be other similar issues. We've not attempted complete coverage, just what's needed by internal use cases. No review pending but feel free to raise an issue for it if you think it will be helpful.

[rhattersley]

Ideally, a test would be nice.

[pp-mo]

As I already said 👍 !

But...
I'm currently working to add more tests, and some more time-unit cases.
Also tests more specifically targetted to this code (not using general iris load operations), as suggested by @rhattersley (private communication).
This will be a little while longer, so I'll submit it separately later.

So, don't wait up !

[bblay]

@pp-mo, are you recommending this is merged without said test because it's on it's way in a coming pr?
Sounds reasonable.

[pp-mo]

@pp-mo, are you recommending this is merged without said test
because it's on it's way in a coming pr?

Yes, absolutely. It's needed for a support enquiry, and the other work will take a while yet.

[rhattersley]

Sounds like your future PR has got lots of new tests in? Can we not cherry pick just the one or two that are specific to this change?

[pp-mo]

It's not (just) the coverage, I'm still sorting out the revised testing method and (still) not yet ready to commit it.
The extra functional changes also involved some refactoring of the grib.py code, and I rather failed to keep the two things separate (doh!). So I think I'd now better disentangle the two again before submitting anything.

[bblay]

[rhattersley]

All of these timedelta constructor calls would be much better written using keywords, e.g.:

datetime.timedelta(hours=int(time_diff * 3))

[pp-mo]

Thanks, I'll add this improvement in my upcoming change to support additional unit codes.
The changes will significantly tidy this code anyway, as I discovered the unit conversions can be unified in one place with considerable benefit.

[rhattersley]

There are external time pressures for getting this extension onto master, so I'm going to merge as it currently stands.

[rhattersley]

@pp-mo If you could include the timedelta-using-keywords change mentioned above in your upcoming PR that would be lovely, thank you. :)

[pp-mo]

@pp-mo If you could include the timedelta-using-keywords change mentioned above in your upcoming PR
that would be lovely, thank you. :)

Ok, as noted above.