fix(lcn): empty formula rejection, vars typo, and boolean comparison cleanup - #10
Open
ThinkerDesigns wants to merge 2 commits into
Open
fix(lcn): empty formula rejection, vars typo, and boolean comparison cleanup#10ThinkerDesigns wants to merge 2 commits into
ThinkerDesigns wants to merge 2 commits into
Conversation
The parser used bool() on the string after ";", but bool("False")
returns True since any non-empty string is truthy. This means writing
"; tau=False" in a sentence file silently set tau=True instead of False.
Fix: split on "=" to extract the actual boolean value string, then compare
with "true". Fixes IBM#5 which requested debugging this parsing issue.
…comparisons
- Line 54: \`vars\` was a typo referencing the built-in vars() instead of
the parsed \`atoms\` variable. This silently disabled formula validation.
Empty/malformed formulas would create Formula objects with empty atom names
instead of raising ValueError.
- Added explicit rejection of empty and whitespace-only formulas that parse as
valid but contain no content (e.g., parsing "" returns ("", {"V1": ""})).
- Replaced six occurrences of \`is True\` / \`is False\` comparisons in
generator.py with idiomatic truthiness checks. These work with Python bools
today due to singleton caching, but would silently fail if cpts ever became
numpy arrays (numpy.bool_ is not the same singleton as Python True/False).
There was a problem hiding this comment.
🟡 Not ready to approve
The updated tau parsing logic can raise IndexError for existing .lcn files that use ; True/False (without tau=), breaking parsing of current example inputs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR addresses formula-validation and parsing edge cases in the LCN model layer, and applies small boolean-idiom cleanups in the generator utilities.
Changes:
- Fixes a validation guard in
Formula.__init__(checkingatomsrather than the built-invars) and rejects empty/whitespace-only formulas. - Updates
from_lcn()sentence parsing to interpret thetausuffix as a real boolean value instead ofbool(non_empty_string). - Replaces
is True/is Falsechecks with idiomatic boolean checks inlcn/generator.py.
File summaries
| File | Description |
|---|---|
| lcn/model.py | Tightens formula validation and updates tau parsing in .lcn sentence ingestion. |
| lcn/generator.py | Removes singleton-boolean comparisons for clearer, idiomatic conditionals. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+900
to
+904
| tau_str = line[pos+1:].strip() | ||
| # Parse "tau=true" or "tau=false" correctly — bool() of any | ||
| # non-empty string (including "False") returns True, so we | ||
| # must split on "=" to get the actual boolean value. | ||
| tau = tau_str.split("=")[1].lower() == 'true' |
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.
Covers three bugs found during code audit:
1. High severity - vars typo disabled formula validation (line 54)
The variable was renamed from vars to atoms in a prior refactor but the validation line wasn't updated. vars() always returns a dict, so this check silently passes for malformed formulas, creating Formula objects with empty atom names.
2. Medium severity - empty/whitespace-only formulas accepted
Empty strings parse as valid and propagate invalid data through Sentence classes downstream.
3. Low severity - is True/is False comparisons in generator.py
Six occurrences of singleton bool comparisons that work today due to CPython caching but would silently fail if the arrays were ever changed to numpy.bool_. Changed to idiomatic truthiness checks.