fix(Niger): the merge_how comment claims an effect it does not have#661
Open
ligon wants to merge 1 commit into
Open
fix(Niger): the merge_how comment claims an effect it does not have#661ligon wants to merge 1 commit into
ligon wants to merge 1 commit into
Conversation
…t it does not have Found by the adversarial re-verification of PR #643, which measured the claim false and correctly noted it is PRE-EXISTING on development (landed by 3488b79), not introduced by that PR — so it is fixed here rather than blocking it, and it would otherwise ship in the next release. The comment said `left` is "load-bearing" because the offsets file's one trailing null-grappe row "arrives as a phantom cluster with a null v" under the default `outer`. Measured: built with and without `merge_how: left`, the table is md5-identical (1,599 rows; 2011-12 at 270 clusters, 0 null v). Under `outer` the row does not survive — the (t, v) groupby drops it. `left` is kept: it states the intent (df_main is authoritative for which clusters exist) and would matter if downstream null-key handling changed. Only the justification is corrected. This is the same defect class the whole #323 sweep has been finding all day — a confident, specific, false statement in a tracked file. It is worse here than in a doc: `data_info.yml` is the file the next editor of this wiring actually opens, and the corrected version already exists in `Niger/_/CONTENTS.org` and the ledger, so the true and false versions were coexisting in one country tree. Comment-only; `Country('Niger').data_scheme` parses unchanged (23 tables). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by the adversarial re-verification of #643, which measured the claim false and correctly noted it is pre-existing on
development(landed by3488b791), not introduced by that PR — so it is fixed here rather than blocking it, and it would otherwise ship in the next release.The claim
Measured
Built with and without
merge_how: left: the table is md5-identical (1,599 rows; 2011-12 at 270 clusters, 0 nullv). Underouterthe null row does not survive — the(t, v)groupby drops it.leftis kept. It states the intent (df_mainis authoritative for which clusters exist) and would matter if downstream null-key handling changed. Only the justification is corrected, and the comment now says defensive rather than load-bearing.Why it is worth a PR
Same defect class the #323 sweep has been finding all day — a confident, specific, false statement in a tracked file. It is worse here than in documentation:
data_info.ymlis the file the next editor of this wiring actually opens, and the corrected version already existed inNiger/_/CONTENTS.organd the ledger. The true and false versions were coexisting in one country tree, with the false one in the config.Comment-only;
Country("Niger").data_schemeparses unchanged (23 tables).Refs #643, #627, #323.