[BUGFIX] Incorrect HTML report description for ExpectTableColumnsToMatchSet#11719
[BUGFIX] Incorrect HTML report description for ExpectTableColumnsToMatchSet#11719wavebyrd wants to merge 3 commits intogreat-expectations:developfrom
Conversation
|
| Name | Link |
|---|---|
| 🔨 Latest commit | 82296c6 |
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: carson@carsons-macbook-pro.local |
1 similar comment
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: carson@carsons-macbook-pro.local |
4c9de3c to
0bb7b84
Compare
|
Hi @wavebyrd,
|
…_match not set When exact_match is omitted (defaults to True), both prescriptive renderers incorrectly resolved the description to "at least" instead of "exactly". The atomic renderer failed because it checked `params.exact_match and params.exact_match.value is True`, which short-circuits to False when exact_match is None. The legacy renderer checked `params["exact_match"] is True`, which also fails for None. Fixes great-expectations#10978
0bb7b84 to
392ca1f
Compare
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: wavebyrd |
|
Fixed the branch history - it now contains only the single commit for this fix. I'll sign the CLA shortly. @cla-bot check |
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: wavebyrd |
for more information, see https://pre-commit.ci
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: wavebyrd |
billdirks
left a comment
There was a problem hiding this comment.
Thanks, this looks good. I've requested you add a comment explaining the root cause of the issue and the need for this fix.
| @@ -286,7 +286,9 @@ def _prescriptive_template( | |||
| ) | |||
|
|
|||
| exact_match_str = ( | |||
There was a problem hiding this comment.
This change looks good but it wasn't apparent to me at first because the bug is caused by something happening in Expectation.configuration. Fixing that would be a bigger change so could you add this comment explaining why this is necessary?
| exact_match_str = ( | |
| # params.exact_match may be None because Expectation.configuration serializes kwargs | |
| # with exclude_defaults=True. Since exact_match defaults to True, None should be | |
| # treated as True here. | |
| exact_match_str = ( |
| ) | ||
|
|
||
| exact_match_str = "exactly" if params["exact_match"] is True else "at least" | ||
| exact_match_str = "exactly" if params["exact_match"] is not False else "at least" |
There was a problem hiding this comment.
| exact_match_str = "exactly" if params["exact_match"] is not False else "at least" | |
| # See comment in _prescriptive_template — exact_match may be None | |
| # when its value matches the class default (True). | |
| exact_match_str = "exactly" if params["exact_match"] is not False else "at least" |
|
@wavebyrd I've left a review. Overall the fix looks good. Could you sign the CLA (see message above)? You will need to use the email associated with your github account for it to be accepted. |
|
A new contributor, HUZZAH! Welcome and thanks for joining our community. In order to accept a pull request we require that all contributors sign our Contributor License Agreement. We have two different CLAs, depending on whether you are contributing to GX in a personal or professional capacity. Please sign the one that is applicable to your situation so that we may accept your contribution: Individual Contributor License Agreement v1.0 Once you have signed the CLA, you can add a comment with the text Please reach out to the #gx-community-support channel, on our Slack if you have any questions or if you have already signed the CLA and are receiving this message in error. Users missing a CLA: wavebyrd |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #11719 +/- ##
===========================================
- Coverage 84.59% 84.19% -0.40%
===========================================
Files 473 473
Lines 39260 39260
===========================================
- Hits 33211 33054 -157
- Misses 6049 6206 +157 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
exact_matchis not explicitly passed toExpectTableColumnsToMatchSet, it defaults toTrue, but both prescriptive renderers resolved the description to "at least" instead of "exactly"params.exact_match and params.exact_match.value is True, which short-circuits toFalsewhenparams.exact_matchisNoneparams["exact_match"] is True, which also evaluatesFalseforNoneNone(the default) the same asTrueFixes #10978
Test plan
ExpectTableColumnsToMatchSetwith noexact_matchargument renders "Must have exactly these columns" in the HTML reportexact_match=Truestill renders "exactly"exact_match=Falsestill renders "at least"