Add write option for geopackage and geoparquet

[val-ismaili]

I've add the option to write to geopackage or geoparquet. default is changed from .geojson to .gpkg

the new cli command looks like:
osmox run configs/example.json example/isle-of-man-latest.osm.pbf isle-of-man -f geopackage -crs epsg:27700 -l

To allow write to geoparquet I had to pip install pyarrow. What's the best way of updating the environment for the repo?

[brynpickering]

Looks good. A range of suggestions for making things a bit slimmer and possibly more maintanable (fewer places to make changes in future).

Also, this would be a good opportunity to add a CLI test. You can check out how it's done in pam: https://github.com/arup-group/pam/blob/main/tests/test_99_cli.py (you basically create a dummy CLI call and then check that it did what you expected it to).

Note that when storing data, the tests do so to a temporary path tmp_path. This is a pytest fixture: https://docs.pytest.org/en/6.2.x/tmpdir.html

[brynpickering]

RE pyarrow: add it to requirements/base.txt. This is the file containing all dependencies that the module depends upon. You should set some loose pinning according to the latest pyarrow release (pyarrow >= 15, < 16?)

[val-ismaili]

@brynpickering I've refactored cli.py as your suggestions (I think!). Taken a stab at testing cli. Figured the two things to test would be:
a) by excluding optional arguments, it uses the correct defaults, and;
b) each of the three output format types work

The tests currently both fail...and I'm not sure what's going on...

[brynpickering]

Minor comments.

Understandable that you're having trouble with the CLI test debugging - it's always a bit of a pain. I've pushed a change with a helper function to add the full error traceback. You can hopefully use that traceback to debug the issue (something to do with the progress bar helper function).

Another point: don't forget to install pre-commit in your environment to have the code formatted correctly on commit (pre-commit install).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.65217% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 82.44%. Comparing base (983f8c0) to head (a9d53b0).
Report is 2 commits behind head on main.

Files	Patch %	Lines
src/osmox/cli.py	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main      #42       +/-   ##
===========================================
+ Coverage   63.75%   82.44%   +18.69%     
===========================================
  Files           5        5               
  Lines         480      490       +10     
  Branches      116      118        +2     
===========================================
+ Hits          306      404       +98     
+ Misses        154       61       -93     
- Partials       20       25        +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[brynpickering]

Great @val-ismaili ! Now just need to run pre-commit install and pre-commit run --all in your local repo so that it formats all the files to make the pre-commit.ci bot happy. This will make sure that files you previously committed without pre-commit installed will be retrospectively formatted

[val-ismaili]

@brynpickering yeah i was running pre-commit but still getting the issue. unsure what im missing...

(osmox) val.ismaili@LDNML6TDXNNY osmox % pre-commit run --all
check for added large files..............................................Passed
black....................................................................Passed
ruff.....................................................................Failed
- hook id: ruff
- exit code: 1
- files were modified by this hook

Found 4 errors (4 fixed, 0 remaining).

[brynpickering]

Once you run it, you should have 4 files with changes that you can then commit - right?

[val-ismaili]

i did yes, which is what the previous commit was here (8830df3). now when i run pre-commit run --all it shows all as passing which is why im confused why its failing on the PR.

(osmox) val.ismaili@LDNML6TDXNNY osmox % pre-commit run --all
check for added large files..............................................Passed
black....................................................................Passed
ruff.....................................................................Passed

[brynpickering]

Aha, you probably still have an osmox directory in your local repo (probably empty, maybe ending in egg_info or similar). Ruff is getting confused by that vs the new src/osmox directory structure.

[brynpickering]

I've pushed an update after runnign it on my local repo

[val-ismaili]

Yeah that was it - got it working now. Thanks for bearing with me on all this. Turns out writing the tests weren't too bad, it was the other stuff i was banging my head against 😅 Also updated test_cli.py to use tmp_path so it doesn't write the files to disk.

[brynpickering]

Nice, it's getting there!

Now that you have the hang of the tests, there's just some improvements to make to those that are there and one more I think is worth adding to match your code changes. Hopefully they won't be too difficult to implement.

[val-ismaili]

I've added a test as per your comments for testing default crs is still saved when user enters a custom crs. And then also adding to the existing test. I've put in fixtures where I thought made sense.

[brynpickering]

Looking good!

[brynpickering]

I added a check for file existence when changing the file extension. I've also updated the config now that you've added these tests and raised project coverage from 60% to 84% (@mfitz won't be happy that it is through an integration test rather than unit tests, but we'll let that slide ;) ).