Open victoriaatraft opened 3 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 92.96%. Comparing base (
c503e1d
) to head (880f13b
). Report is 6 commits behind head on develop.
adding blocked tag pending feedback on validator changes.
adding blocked tag pending feedback on validator changes.
okay. I 100% deleted them, so sorry about that. @elipe17 will graciously add them back in so it can be good to go.
@victoriaatraft a few of these friendly names are still quite verbose. lets discuss options for a path forward at next UX sync. cc: @lfrohlich
@elipe17 @victoriaatraft i left comments on a couple of line changes that can be prioritized for merging, perhaps on another branch. defer to dev on how best to go about that.
Thanks @ADPennington! checked in with @elipe17 and will be tackling this, this afternoon and wrap up by tomorrow morning
@elipe17 @victoriaatraft i left comments on a couple of line changes that can be prioritized for merging, perhaps on another branch. defer to dev on how best to go about that.
@ADPennington, @victoriaatraft I pulled the changes into another branch: merge-first-changes
and made that branch this branch's base branch. I tagged everyone on the new PR.
@ADPennington @lfrohlich I know it says change requested, I tried to resolve that but those changes were addressed by Eric :)
@ADPennington @lfrohlich I know it says change requested, I tried to resolve that but those changes were addressed by Eric :)
thank you. just one quick question on this. Any suggestions on an efficient way to test this? For context, all of the friendly names have changed with this PR which currently only appear in the error reports. I usually confirm these sorts of changes by submitting a test file that would yield the relevant error messages that have changed. I'm not sure if we have some test files relevant to this PR to help make this a little easier. @victoriaatraft @reitermb @elipe17
@ADPennington @lfrohlich I know it says change requested, I tried to resolve that but those changes were addressed by Eric :)
thank you. just one quick question on this. Any suggestions on an efficient way to test this? For context, all of the friendly names have changed with this PR which currently only appear in the error reports. I usually confirm these sorts of changes by submitting a test file that would yield the relevant error messages that have changed. I'm not sure if we have some test files relevant to this PR to help make this a little easier. @victoriaatraft @reitermb @elipe17
@ADPennington there is not an easy way for us to do this at the moment. Best we can do is submit files that have lots of errors, e.g. ADS.E2J.FTP1.TS06
, and do a manual verification. The only way we might be able to actually test this programmatically is if we built a file generator that allowed us to specify if we want to fuzz the values for the record types. Then we could generate files that have an error for every single field in each record type quite easily.
unit tests now failing on this @elipe17 @victoriaatraft
Details
Ah yes, these failures are result of the recent PRs being merged. @ADPennington, @victoriaatraft I will take a look shortly.
@victoriaatraft and @ADPennington I resolved the suggestions/changes that were in my purview. I left a few open for Victoria/@reitermb to address.
@elipe17 @ADPennington resolved open suggestion and made the last to Alex's open comment. We're removing "Race/Ethnicity" thank you
Summary of Changes
New PR to correct for some unwanted conflicts with develop Closes #2801