Fix use_dual type hints#1612
Conversation
The public grid-loading APIs document and implement use_dual as a boolean, but their annotations also accepted None. Align the public and internal signatures with the actual contract and add focused regression coverage.
|
Thank you for these contributions, @Sanjays2402! This looks like a clean fix to the None type hints on the use_dual kwarg, and one good way to test things. I have a few thoughts/notes before we get this merged into main:
Also, out of curiosity, @Sanjays2402 what led you to uxarray? It is always helpful to hear how people find out about the project! |
|
Yeah, I've been noticing this too, seems like an anti-pattern to allow both |
|
Agreed on the scope, "fixes part of #1593" is the accurate framing, On the test: I'd lean (B). It's spot-check by nature and I share your concern about setting an expectation that every kwarg gets a hand-written type-hint test, that gets unmaintainable fast and the test does key off hint spelling. Happy to add a comment saying as much. If the team would rather go (C) I won't argue, the signature fix stands on its own without it. Say which way you want it and I'll push. |
Fixes part of #1593 but does not fully close it.
Overview
use_dualtoboolacross the public grid-loading APIs andGrid.from_dataset, matching the documented and implemented contract.PR Checklist
General
Testing
Documentation
_) and have been added todocs/internal_api/index.rstdocs/user_api/index.rstExamples
docs/examples/folderdocs/examples.rsttoctreedocs/gallery.ymlwith appropriate thumbnail photo indocs/_static/thumbnails/