Write bindings between shapefile API and product-metadata

[jackrosacker]

See #1785

todos

• Write write_shapefile_xml_metadata() or equivalent function (see notes)
• Write effective test (make sure to test a field value, not just presence of metadata)

[codecov]

Codecov Report

❌ Patch coverage is 98.17073% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.36%. Comparing base (aef37fb) to head (c9ad19e).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
dcpy/lifecycle/package/shapefiles.py	94.11%	1 Missing and 1 partial ⚠️
dcpy/utils/geospatial/shapefile.py	92.85%	0 Missing and 1 partial ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
dcpy/lifecycle/product_metadata.py	100.00% <100.00%> (ø)
dcpy/models/data/shapefile_metadata.py	100.00% <100.00%> (ø)
dcpy/utils/geospatial/shapefile.py	97.75% <92.85%> (+0.05%)	⬆️
dcpy/lifecycle/package/shapefiles.py	93.15% <94.11%> (+0.65%)	⬆️

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jackrosacker]

Hiya @alexrichey / @damonmcc - I think this is ready for a high level check-in to make sure I'm taking this in a suitable direction.

A few notes/questions/decisions:

• I've made various changes directly to the pydantic-xml model. I know that the docstring at the top says to not edit the file directly, but I needed to tweak it slightly to get it to work the way I needed it to
  • This begs the question - assuming we stick with the same approach to editing the model generator code, how should I track prompt/specification changes as I tweak the output model? i.e. one of the changes I made it to ensure that every nested sub-class of the model is instantiated, even when not values are provided. This would presumably change the original specification of the model generation code, and I should probably be logging it somewhere? Happy to follow your lead on this @alexrichey, or to make my own call and create a prompt file somewhere.
• Decided to migrate default model values into the pydantic-xml model itself, instead of having them live in the generate_metadata() code
• Note that the typer CLI requires a bunch of arguments that get passed directly to the underlying function. Does this feel right? This is also potentially an opportunity to resurrect some of my initial code that would allow a user to pass an entire path to the CLI (e.g. "path/to/file.zip/subdir/another_subdir/shapefile.shp"), and have helper code break it up into constituent path (path/to/file.zip), shp_name (shapefile.shp), zip_subdir (subdir/another_subdir) etc components. I don't know if this is helpful and simplifies the interface, or if it adds unnecessary complexity
• There's currently no way to differ from the field assignments defined in the top level function. I think at some point it will be important to allow fields to be ignored/overridden/provided as unique values, but chatting with @damonmcc the other day made it sound like that can wait until we actually need it

Assuming this is the right direction, my next steps will be to:

• Add the remaining fields to the top level function, in order to fully populate MapPLUTO metadata as it currently exists
• Make sure that the current approach works for the dataset column definition fields in the model
• Fix the failing tests -> looks like typer isn't recognizing OrgMetadata as a class?

[alexrichey]

@jackrosacker This is great - everything seems like it's right where it ought to be. Quick thoughts on your questions:

• For model changes: I think you can change with wild abandon, and tracking with commit messages is enough. Ideally, you'd have a separate commit for the model change, and in the body of the commit, you can write out your reasoning. But for now, since nothings actually released, I wouldn't worry about that sort of thing.
• "Note that the typer CLI requires a bunch of arguments that get passed directly to the underlying function" It's not ideal, but I think it's fine.

[jackrosacker]

@alexrichey ok understood. Thanks for taking a look!

For the model changes + proper git messages - are you imagining these commits would persist after rebasing, in order to keep track of model changes over time to edit the original prompt, or are you just thinking your suggestion would be to help with reviewer understanding, and those commits would get squashed with all the others when rebasing.

[jackrosacker]

Ah, and one other question for whoever looks at this next: realizing I'm not testing the typer CLI commands. Should I add tests for the CLI specifically, or are those handled differently from the tests for the underlying functions?

[alexrichey]

@jackrosacker

are you imagining these commits would persist after rebasing

In the ideal world they do. You can rebase the commits to preserve those with important documentation, then rebase the entire branch. I wouldn't worry too much about it now, but let me know if you want a hand with that.

Should I add tests for the CLI specifically

Great question - we don't test those similarly to the rest of our code. We'll typically rely on something much more akin to integration tests. We have a dummy product that we build called template-db where we try to invoke relevant typer CLI commands. We can worry about that later, I think.

[jackrosacker]

@alexrichey / @damonmcc I've committed what I think is a fairly complete initial run at this PR. Recent changes added column level metadata that is populated when relevant. Ready for some review!

[alexrichey]

Looks great! had one change for the CLI arg for the org md path, but when you fix that, feel free to merge (after Damon takes a look, of course)

Also, when you merge, make sure you squash.