Add demographic data collection methods for race, ethnicity, age, income, and occupation #974
Conversation
Co-authored-by: nonprofittechy <[email protected]>
Copilot
AI
changed the title
|
Look at #971 and make sure the new methods support the Add a test_demographics.yml file that lets you interactively explore this new feature. Look at existing test_... YML files for structure. Instead of hardcoding strings in the methods, follow the pattern of |
…d test file Co-authored-by: nonprofittechy <[email protected]>
I've addressed all the feedback points:
The implementation now follows the existing patterns consistently and provides the same level of customization as other field methods. Changes in commit 79be9d1. |
|
@copilot update the docstrings for the new methods so they explicitly reference the new hidden |
Co-authored-by: nonprofittechy <[email protected]>
Updated the docstrings for all four demographic methods to explicitly reference the
Each docstring now includes the specific variable name in the |
|
@copilot review the unittest errors. Importantly, ......................E.EEEEEEE.........ERROR: test_age_range_fields_basic (AssemblyLine.test_al_general.test_demographic_fields.test_age_range_fields_basic)
|
| choices = choices() | ||
|
|
||
| other_input = { | ||
| "label": "Please specify", |
There was a problem hiding this comment.
For translation, I think the best option is to pass in templates for this sort of UI text?
It is really cumbersome to do though, but I know you've said that translation with excel is easier than the words.xml.
There was a problem hiding this comment.
What do you mean by pass in templates? do you mean make the parameter a template: ... block? I don't think that's very helpful because you need to consistently use the same parameter with every call to the method. That doesn't make sense if there's just one standard phrasing that isn't likely to change.
Good catch on this not being translatable though--I will make it consistent with other places where we use an attribute of the object.
| if maxlengths: | ||
| for field in fields: | ||
| if field["field"] in maxlengths: | ||
| field["maxlength"] = maxlengths[field["field"]] |
There was a problem hiding this comment.
Does docassemble error when you put a maxlength of a non-text field? The only maxlength field here is the other option, right?
| {"She/her/hers": "she/her/hers"}, | ||
| {"They/them/theirs": "they/them/theirs"}, | ||
| ] | ||
| if variable_name == "al_race_ethnicity_choices": |
There was a problem hiding this comment.
Since the demographic questions are being tested in a different class, can these extra default definitions be removed?
There was a problem hiding this comment.
I'm working separately on the tests; having CoPilot generate wasn't helpful. I might remove them.
Good feedback - I'm in a testing phase now. I think we can use the instruction file to help with that; also I'm happy to just close a PR that has too many issues and try manually later. I'm mostly trying to use CoPilot to take care of very small and easy to read PRs or things that were on our to-do list for a very long time. Nothing where giving a review should be a blocker--if anything, reviewing within a week would be way faster than waiting months/years for the fix to get added manually. You should not feel obligated to review [any PR adding a new feature] quickly--if you want a chance to review before merging, I'm happy with a delayed review pace [and I can slow down my merging pace]. |
I would appreciate slowing down the merging pace, getting back on GitHub and seeing like 10 PRs merged was a bit of a jump scare, especially after glancing through the first few in the docs repo and seeing some pretty noticeable errors. But the PRs to the code repos haven't been as bad, I'm guessing because you're working through them more. I do get a little nervous when the PRs are more than 100 lines or so, but I guess for some of these features it'll need to be. |
|
I don't think that was a copilot error although I could look at the git blame. I think it was my manual change (copilot does run the build step and actually shows screenshots, so it would normally do better). Mostly what I'm seeing is it going too far with changes or adding irrelevant sources, not typographical or syntax errors. |
3 participants
requiredparameter to _fields() methods for granular field requirement control #971Summary of Latest Changes:
Updated all demographic method docstrings to explicitly reference the
al_*_choicesvariables defined inql_baseline.yml:race_and_ethnicity_fields()now referencesal_race_ethnicity_choicesage_range_fields()now referencesal_age_range_choicesincome_range_fields()now referencesal_income_range_choicesoccupation_fields()now referencesal_occupation_choicesEach docstring now includes:
choicesparameter description mentioning the specifical_*variable used as defaultal_*_choicesvariable inql_baseline.ymland can be customized by redefining the variableThis makes the customization pathway more discoverable for developers using these methods.
The implementation now follows all existing patterns and supports the full range of customization options available in other field methods.
Fixes #455.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.