search

opengeos / open-buildings

Tools for working with open building datasets
https://opengeos.github.io/open-buildings
Other
119 stars 17 forks source link

Add a few tests, refactoring #44

Closed felix-schott closed 8 months ago

felix-schott commented 8 months ago

Hi @cholmes,

As promised, I added a few tests (#36) and refactored the code a bit. I prefer to work in containers, so I added a Dockerfile for developing as well. When working in a container, you can avoid things like the issue Darren reported (DuckDB spatial extension not installed) as you develop in a more controlled environment. If you're on linux, you can simply run the dev-container.sh script or make use of the VSCode container extensions.

Apart from adding tests, I changed a few things:

Sorry for overloading this PR, some changes could have arguably been a separate PR. There are more tests that could/should be added, I focussed on the download function for now (might add more in the future).

Also, I updated the CI/CD pipeline - I'm not too familiar with Github actions and wasn't sure how to test, so not guaranteed that this will work (we'll find out).

One important thing: I added tests for all the different data formats. Downloads for all pass except for shapefile - at least in my environment. Not sure if this is a bug or something wrong with my setup? Has this ever worked? Hash out the if != shapefile bit in the test to reproduce.

Thanks for reviewing these changes, let me know if anything is unclear. Felix

felix-schott commented 8 months ago

I created a bug report for the shapefile issue (#48), I believe this can be dealt with in a separate PR as it doesn't seem to be related to the test setup.

cholmes commented 8 months ago

I prefer to work in containers, so I added a Dockerfile for developing as well.

cholmes commented 8 months ago

So looks like 'match' is syntax from 3.10, and our test suite does 3.8 and 3.9. I have no idea about the python ecosystem, and how important it is to support older versions (and how many people are on like 3.10 and above), so I'm probably fine to just remove those tests if 3.8 and 3.9 aren't worth the tradeoffs, but I defer to those who know Python better.

cholmes commented 8 months ago

I prefer to work in containers, so I added a Dockerfile for developing as well.

Great - I'll try out working that way, it makes lots of sense to me.

Sorry for overloading this PR, some changes could have arguably been a separate PR.

It's all good - I think as the project matures we can get more disciplined about tighter PR's, but since we don't even have tests yet and this adds them I think it is all good.

There are more tests that could/should be added, I focussed on the download function for now (might add more in the future).

Very reasonable - we can try to start pushing any new PR to have tests, and ticket some test making that could be good 'first issues' for people interested in joining.

Also, I updated the CI/CD pipeline - I'm not too familiar with Github actions and wasn't sure how to test, so not guaranteed that this will work (we'll find out).

Sounds good - happy to YOLO this and see what happens.

cholmes commented 8 months ago

@felix-schott - just gave you increased rights on the repo, as I'd love you reviewing PR's, as I'm sure I'll learn a lot having you look over my code. No worries if you don't want to at any point, but wanted to give you the rights to do so.

felix-schott commented 8 months ago

i changed the | syntax to the older Union syntax - sorry for the mess, I'm used to developing in more recent Python versions. i had also added the shapefile test back and it turns out it seems to be a general linux problem as it also failed in the pipeline! anyway, i gotta leave now, will check back in tomorrow if there's still issues (looks like the ci/cd run needs to be approved by you).

thanks for giving me increased rights! happy to review changes :)

cholmes commented 8 months ago

Awesome, thanks! I'm going to try to figure out why I need to approve every workflow. I was hoping that giving you more rights would at least let you run it. Hopefully I can find something more liberal.

cholmes commented 8 months ago

Ok, made it more liberal, to 'Only first-time contributors who recently created a GitHub account will require approval to run workflows.' so hopefully this will work by default more. It's definitely annoying for me to come to a new PR and then have to hit 'run' and wait, and clearly would be more helpful to a first time contributor to be able to see the results of CI.

felix-schott commented 8 months ago

@cholmes All tests passed - I took the liberty and merged since you previously signalled approval. Hope that's okay. Probably doesn't need a new tag since it doesn't add any major features, just tests and some refactoring. The only feature would be that directories are permitted as dst. Thanks for your time reviewing these changes :)

cholmes commented 8 months ago

Definitely ok to merge, thanks!