Add unittests to define basic functionality for templates

aodn / python-aodntools

Repository for templates and code relating to generating standard NetCDF files for the Australia Ocean Data Network

GNU Lesser General Public License v3.0

10 stars 3 forks source link

Add unittests to define basic functionality for templates #3

Closed mhidas closed 6 years ago

mhidas commented 6 years ago

WIP

ocehugo commented 6 years ago

Looks good!

Comments:

So it's python2 all the way!? I can see some py2 changes that are not py3 compatible!? (only the super call I believe).
What about the template testing? How far the json validator can do the test for us? For example, a check for negative dimensions in the template. Can the validator do this? I believe if we put a string in dimensions values, the validator should pick up, but can it pickup a negative dim?

I can push some invalid json templates based on invalid cdls if you want.

ghost commented 6 years ago

We are not too far away from tackling the Python 3 compatibility work for everything, so

https://github.com/aodn/backlog/issues/908

Using super with arguments is backwards compatible in Python 3, so should work fine in both versions with arguments: https://docs.python.org/3/library/functions.html#super

mhidas commented 6 years ago

What about the template testing?

Yes, I'll add that. We can add specific checks for things like negative dimensions.

I can push some invalid json templates based on invalid cdls if you want.

Thanks, a couple of those could be helpful.

mhidas commented 6 years ago

I think this is now at a point where we could review & merge.

There's definitely more work to do afterwards, including:

Further validation of the template before creating the file (@ocehugo's code does that, but I think with the new jsonschema approach we can simplify a lot of it).
Making sure we're compatible with Python 2 & 3.
Packaging it all up!
adding more functionality
etc, etc...

lbesnard commented 6 years ago

can you please add in the README.md the basics on how to use this package, how does it work ...

mhidas commented 6 years ago

Thanks @lbesnard. I was focusing on getting the thing functional, so haven't put much effort into documentation, but I will start adding a few more comments, and update the README.

Also, now that I have tests written, there's a fair bit of refactoring I want to do in the template.py code, so I'll add comments after that.

mhidas commented 6 years ago

For a start I've renamed DatasetTemplate.create() method to to_netcdf() and expanded its doc string. I've also added a very quick example of usage to the README.md. This can be further improved later.

mhidas commented 6 years ago

the function names/variables need to be consistent

I agree, but I was planning to do that as part of the refactoring and cleaning up, which I'd like to do in a separate PR, if that's ok. The main purpose of this PR was to define the unittests and get the existing code to pass them with minimal changes.

mhidas commented 6 years ago

Thanks for the comments. Could we merge this PR, with the understanding that it's still a work in progress, and further improvements, refactoring and documentation will come soon in later PRs?

ocehugo commented 6 years ago

I don't see why it was not merged yet :)

lbesnard commented 6 years ago

it didn't really matter as there is only one PR/branch in this repo, and it leaves some space for discussion like this thread.

I guess the thing we didn't really talk enough about is what do we want the class to output. writing a netcdf file, sure, but imo I'd like the class to keep the netcdf object open for further writings. It doesn't seem to be the case from this line https://github.com/aodn/aodn-netcdf-tools/blob/master/ncwriter/template.py#L442