Feature/unit tests

[sdat2]

Hi,

I am adding a some unit tests that will run as a github action for a few different versions of python at each push.

Currently it only pip installs the package and creates the test data, but I will add a few more.

I've added settings in the pytest.ini file that allow code blocks using >>> in python docstrings to act both as unit tests and examples, which should hopefully reduce duplication.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[sdat2]

python-3.10 not able to install rasterio as detailed in #71 .

[herbiebradley]

Feel free to unpin rasterio from the requirements, then run the GitHub actions again! I tested with Python 3.10 and then just pip installed the requirements.txt as normal with the latest version of rasterio and everything seems to work.

[sdat2]

Hi Herbie, That makes sense, I'll try unpinning the rasterio version.

RE using the requirements.txt file, I guess that that isn't an ideal thing to do if we want to make sure our pip package is compatible? Correct me if I'm wrong, but if you pip install geograph from somewhere else, that wouldn't be used? If that's right, that would undermine people's ability to easily build their packages on top of ours.

[sdat2]

Ah ok, so everything seems to be ok now for linux/mac, but I should probably fix windows as well for completeness.

[herbiebradley]

Hi Herbie, That makes sense, I'll try unpinning the rasterio version.

RE using the requirements.txt file, I guess that that isn't an ideal thing to do if we want to make sure our pip package is compatible? Correct me if I'm wrong, but if you pip install geograph from somewhere else, that wouldn't be used? If that's right, that would undermine people's ability to easily build their packages on top of ours.

Yes, I just meant that for dev purposes I use the requirements.txt. Of course the actual place to unpin is setup.py, as you have done. :)

[herbiebradley]

Regarding the GDAL Windows issue, you may be able to fix this by including a GDAL wheels download and install in the configuration somehow: https://stackoverflow.com/questions/55583234/installation-fails-for-fiona-and-geopandas-with-gdal-on-python-3-6-on-microsoft

But it's not a big deal for now if it doesn't work - interestingly this was so annoying for geopandas that they don't try to solve it for the user and instead suggest Windows users use pyogrio which is a fiona alternative.

[herbiebradley]

What do these error codes mean?

[sdat2]

Hi, I'm not sure, I originally adapted this from a github actions example. Given that we have pylint anyway, I will get rid of this section.

[herbiebradley]

Looks good! I was wondering what is the reason for adding pipwin and descartes to setup.py? You could also add matplotlib and seaborn since I noticed some of our code requires them and I couldn't figure out why we didn't include them, unless you can remember?

[sdat2]

Hi, sure, I'm very happy to give up on Windows. I guess we can direct any windows users to this stack exchange answer: https://stackoverflow.com/a/65082853 if they want to use this package.

[sdat2]

Hi Herbie, I only added 'pipwin' and 'descartes' to the setup.py to try to fix the GDAL error based on some stack exchange answers. I will remove them again in the next commit.

[sdat2]

(The CI github action was still triggered by master rather than main)

[sdat2]

I've added only one unit test, which is more of a "smoke test". It might be worth adding some more before we merge this branch in.

[sdat2]

Ah ok, the CI pylint test is broken, do you think that would be worth fixing?

[herbiebradley]

Looks good! I don't think it's necessary to fix the pylint errors for this PR, I can either fix them or turn off the unnecessary error codes later.

[sdat2]

Sure, I can merge and then add some more unit tests in a future pull request.