Pull Request: TypeHinting and Readability

[xaviernogueira]

Hello xpublish-opendap devs / @abkfenris.

First off, great job making a very useful adaptor. I look forward to developing with it and sharing our work with the wider Xpublished community in the near future.

TLDR: I made no functional changes to the repo code at all! Behavior was not modified in any way. I hope you consider reviewing and pulling in these edits, as I'd argue they improve the readability and understandability of the code, which only encourages more use and contributions.

Readability edits:
As I began working with the repo, I noticed readability could be improved to match the standards set by the Google Python StyleGuide as well as PEP8 (i.e., column limits, #-out-able imports/function-arguments, relative "." imports, spacing between functions, etc.).

TypeHinting Edits:
TypeHinting was somewhat inconsistent (missing for a few functions) and personally I am a big fan of "idiot-proofing" repositories via very explicit TypeHinting (including in-line hinting for variables returned by functions).

An example of this to illustrate my thinking where I attempt to make clear the underlying type shared by all opendap_protocol datatypes:

# BEFORE
dtype_dap = {np.dtype(k): v for k, v in dtype_dap.items()}

# AFTER:
dtype_dap: Dict[np.dtype, dap.DAPAtom] = {
    np.dtype(k): v for k, v in dtype_dap.items()
}

All my edits are incredibly minor, but since I hope more people use this code going forward, adding that final but of polish seems worth it.

Thank you for your consideration!

[abkfenris]

Hi @xaviernogueira, thanks for this work! Did you use any tools to help with type hinting or formatting? It would be nice to be able to add those to the pre-commit config so that others can build on your work, or at least not unintentionally revert it.

It looks like I left the tests and pre-commit config broken, so I just made a new PR to attempt to fix them #4, so you may want to merge or rebase onto it (once I have that not-broken).

[xaviernogueira]

@abkfenris I just did the edits manually. That said, I have autopep8 format my code by default in my IDE which would be great for a pre-commit. That said, it doesn't do all that much, mostly clears out trailing spaces and extra empty lines.

How about you ping me when you fix the tests and I can merge to that?

[abkfenris]

Ah ok. I've got Black running as part of the pre-commit hooks which covers formatting. Mypy and Ruff are also both part of the pre-commit hooks too to help with typing and linting, along with a handful of other checks.

I've landed my PR with updated tests and pre-commit config, so you can try merging that/main in.

[xaviernogueira]

Okay I did it! Seems like everything is aligned. Now the tests need to run?

[abkfenris]

I just triggered the tests.

[abkfenris]

It looks like that merge didn't quite do everything cleanly which is causing pytest to fail.

It also looks like your autopep8 config is conflicting with black's auto formatting, which is causing the pre-commit action to fail. Are you able to disable auto-format on save, and let pre-commit run before pushing? (you may need to manually run pre-commit run --all-files to bring things back into sync)?

[xaviernogueira]

All tests passed (manually ran). Some reformatting was done automatically, and I turned off autopep8. The workflow should work now I believe.

[xaviernogueira]

On a side note the documentation seems outdated (no "dap_router", and you would want to register OpenDapPlugin as a plugin not a router)? I can add to that later when I have a moment, but will keep that in a separate pull request.

[abkfenris]

Docs are definitely out of date, so any help with them would be appreciated. They haven't been a big priority yet, as I haven't set up the library to be published to PyPi, and only having a git install helps folks understand 'here be dragons'.

I'm working on a big docs overhaul for Xpublish itself xpublish-community/xpublish#180 which is part of what I want to get done before cutting the first plugin release. Then I'd like to get -opendap and -edr to points that they can be released on PyPi & Conda-Forge too.

[abkfenris]

Great, those passed. Thank you Xavier!