New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add a single command for handling both code linting and formatting. #871
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would want Max's thinking on the changes to HouseholdPage before feeling comfortable "signing off," so to speak. The reason fips fails is because it was a variable that every household had and its default value was 6. The variable was removed around Oct. 31, but because it had a truthy value, the code we used to update households should prompt every household created prior to Oct. 31-ish to re-create their household. It doesn't at the moment because issue #762 breaks the interstitial that's supposed to appear, making everything look okay. #762 should be resolved with PR #867. The front-end test fails at the moment because it pulls a real household for testing purposes, and since every US household had fips, it fails.
So here's what I'm thinking. We could merge this code, which will prevent household invalidation across the board and will make the test pass, but is a bit of an ugly workaround because we'll have to do it any time a variable like fips is deleted in the future. We could re-engineer the household object the test uses to make it hardier, but that will still invalidate all households. And/or, moving forward, we could have a discussion about how better to treat deleted truthy variables in this particular component.
Curious to hear @MaxGhenis's thoughts on this.
|
Even though fips (numeric US state code) had a truthy value (6 = California), the questionnaire should have always overwritten it, since it requests the user's state. And state code/name (e.g. CA) should have still been in there. So I think it's OK to remove fips from the database as long as we retain other state fields. |
|
Also would suggest scoping PRs to more specific issues in the future, e.g. as we're discussing the lint/format change in #902 |
|
@MaxGhenis Fair points on both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was already solved and implemented via #902.
fixes #865
npm run lintnow can handle both linting and formatting without the need for an additional Prettier command.