Docstring rework for _resolve_wildcards, distilled into a CLAUDE.md guide - #135
Docstring rework for _resolve_wildcards, distilled into a CLAUDE.md guide#135nick-gorman wants to merge 2 commits into
Conversation
… guide Review feedback on #126 found the _resolve_wildcards docstring confusing — both its structure and its coined terminology (wildcardable, designed drops). Rewording it produced a set of transferable lessons: established terminology only, a narrative structure that motivates the input's oddities before the mechanism, precision about compound cases, and verifying provenance claims against call sites. These are now captured in CLAUDE.md as a docstring guide for generic helpers, and demonstrated on a second function in the same situation, _fuzzy_match_names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
The greedy loop accepts scores at the threshold (>=), not only above it, and the canonical sub-region names live in mappings.py, not the nodes table they're used to build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EllieKallmier
left a comment
EllieKallmier
left a comment
There was a problem hiding this comment.
Have percolated a bit and left thought comments but in the end I think this is totally reasonable and not a huge deal if not 'perfected' at any point of our process :)
| specific row and a wildcard one. The row that used the fewest wildcards (the | ||
| most specific) wins; callers rely on the schema's *_resolve_unambiguously | ||
| rule to guarantee there is never a tie. | ||
| """Resolve which row of ``table`` applies to each key combination in |
There was a problem hiding this comment.
I have a few thoughts about this particular docstring, that may or may not be actually useful in a general sense (forewarned):
- It's definitely better described and explains what the function does/doesn't do
- It's still not like super smooth to read out of context - but that is totally fine and I think good, in a way - it's not meant to be read out of context
I do however feel like in this specific case there's just a bit of a disconnect or slight discomfort with phrasing, but that I think could be clarified (for me - this is such a personal observation so hugest grains of salt!!!) by one of the following:
- Formatting the 'derived vocabulary' distinctly, e.g., italics or bold or using single quotes: key | key | 'key' -> to be aggressively obviously holding a specific meaning in this context
- OR: I was thinking that if
key_columnswere a defined input to this function, that would immediately clarify some of the language and what it relates to (to me). With the full understanding that that's prob not the best/cleanest/most efficient (or necessary!!) solution I just wonder if there are some cases where a small refactor to make a function's function more obvious could be a win...
Ok big thought dump, again honestly I think this is just a personal taste thing and really not that deep, I generally prefer these versions of the docstring to their old versions and am happy to green-light this general concept of 'give more context when a helper is multi-use' :)
| """ | ||
| ``` | ||
|
|
||
| ### Docstrings: generic helpers |
There was a problem hiding this comment.
As in my other chunky comment, generally I think I am pro- this approach of requiring more in-depth explanation when a helper services multiple call sites/cases. I also though don't think more text is always going to be the best approach for every case (whatever 'best' means lol).
-> Also I would lean towards adopting an approach to example-writing in docstrings that encourages writing up multiple examples where there are a few distinct edge cases or potentially unclear operations as an accompanying comment :)
2 participants
Follows up Ellie's review feedback on #126 that the
_resolve_wildcardsdocstring was confusing. This PR rewords that docstring, and since the rewording surfaced lessons that seemed transferable, captures them as a new CLAUDE.md section on documenting generic helpers, then tests the guide by getting claude to apply it again to to a similar function,_fuzzy_match_names. Genuinely curious if you think this is a decent improvement across both functions. It's also so pretty verbose so we'd need to watch out it isn't biting too often.