fix minimum counts - #116
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new IsAutogram console output introduces unwanted side effects and GetStatedFrequency still mis-parses singular “colon” without an additional replacement.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adjusts handling of “extended-name” punctuation invariants (notably : and =) so their minimum counts and sentence parsing behave correctly when those invariants don’t appear in the numeric-letter list.
Changes:
- Extend stated-frequency parsing to recognize
colonsandequals, and allow:/=as the counted “character” token. - Add
:/=extended-name mappings inCharExtensionsfor rendering count list entries. - Fix invariant-to-variable baseline propagation by indexing
variableNumericCountsusingMinimumCount(instead ofBaselineCount + 1).
File summaries
| File | Description |
|---|---|
| Autogram/Extensions/StringExtensions.cs | Adds parsing support for :/= and adds a mismatch diagnostic line in IsAutogram. |
| Autogram/CharExtensions.cs | Adds extended-name/plural-name mappings for : and =. |
| Autogram/AutogramConfigFactory.cs | Uses MinimumCount when adding invariant cardinal contributions into variable baselines. |
Review details
Suppressed comments (1)
Autogram/Extensions/StringExtensions.cs:25
GetStatedFrequencyconverts extended names into punctuation so the regex captures the right character. Currently only "colons" is replaced; a singular "colon" (e.g. "one colon") will be mis-parsed as the letter 'c' because the regex captures only the first character after the number word.
lower = lower.Replace("colons", ":'s");
lower = lower.Replace("equals", "='s");
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…am-solver into fix-min-counts
There was a problem hiding this comment.
🟡 Changes recommended
The factory logic still risks unclear failures (including potential out-of-range indexing) and the changed minimum-count behavior isn’t protected by a regression test case for configurations where MinimumCount differs from BaselineCount + 1.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/4 changed files
- Comments generated: 3
- Review effort level: Lite
| if (!wordToNumber.TryGetValue(wordNumber, out int declaredCount)) | ||
| throw new InvalidOperationException(); |
| // add the cardinals of the invariants into the variant baseline. | ||
| var numericCount3 = variableNumericCounts[letter.Index][letter.BaselineCount + 1]; | ||
| var numericCount3 = variableNumericCounts[letter.Index][letter.MinimumCount]; |
|
|
||
| // add the cardinals of the invariants into the variant baseline. | ||
| var numericCount3 = variableNumericCounts[letter.Index][letter.BaselineCount + 1]; | ||
| var numericCount3 = variableNumericCounts[letter.Index][letter.MinimumCount]; |
Uh oh!
There was an error while loading. Please reload this page.