fix: add more info to attr value / statement validity#209
Conversation
🦋 Changeset detectedLatest commit: a11c1f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe pull request adds a new changeset entry for the htmljs-parser package indicating a patch release. In 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/validators.ts (1)
83-97:⚠️ Potential issue | 🟠 MajorInconsistent
isEnclosedhandling for single LF newline.The CRLF branch (lines 90) and the general character branch (line 94) both set
isEnclosed = falsewhen outside any group, but the single NEWLINE branch (lines 83-85) does not. This means a single LF character outside grouping constructs won't mark the expression as non-enclosed, while CRLF would.If intentional, consider adding a comment explaining why. Otherwise:
🐛 Proposed fix for consistency
if (code === CODE.NEWLINE) { + if (isEnclosed && !expr.groupStack.length) isEnclosed = false; parser.forward = 1; parser.activeState.eol.call(parser, 1, parser.activeRange); } else if (
bf800f9 to
a11c1f1
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
==========================================
- Coverage 91.88% 91.87% -0.02%
==========================================
Files 28 28
Lines 1418 1427 +9
Branches 319 323 +4
==========================================
+ Hits 1303 1311 +8
- Misses 53 54 +1
Partials 62 62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/validators.ts (1)
31-48:⚠️ Potential issue | 🟠 MajorBreaking API change: boolean → enum return type.
Line 31–48 changes public return types to
Validity(numeric). Any consumer doing strict boolean checks (=== true/false) or relying on boolean typing will break. If you intend a patch release, consider keeping backward-compatible boolean wrappers (or preserving old signatures) and add new “getValidity” APIs; otherwise, bump major and document the breaking change.
🧹 Nitpick comments (1)
src/__tests__/validate.test.ts (1)
7-78: PreferValidity.*over magic numbers in tests.Hard-coded
0/1/2is brittle if enum order changes and is harder to read. Import and useValidity.invalid|valid|enclosedfor clarity.Proposed refactor
-import { isValidAttrValue, isValidStatement } from ".."; +import { isValidAttrValue, isValidStatement, Validity } from ".."; - assert.equal(isValidStatement("foo + bar"), 2); + assert.equal(isValidStatement("foo + bar"), Validity.enclosed); - assert.equal(isValidStatement("foo\n + bar"), 1); + assert.equal(isValidStatement("foo\n + bar"), Validity.valid); - assert.equal(isValidStatement("foo\nbar"), 0); + assert.equal(isValidStatement("foo\nbar"), Validity.invalid); - assert.equal(isValidStatement("foo ?\n bar : baz"), 2); + assert.equal(isValidStatement("foo ?\n bar : baz"), Validity.enclosed); - assert.equal(isValidStatement("(foo"), 0); + assert.equal(isValidStatement("(foo"), Validity.invalid); - assert.equal(isValidStatement(")"), 0); + assert.equal(isValidStatement(")"), Validity.invalid); - assert.equal(isValidAttrValue("foo + bar", false), 2); + assert.equal(isValidAttrValue("foo + bar", false), Validity.enclosed); - assert.equal(isValidAttrValue("foo=>bar", false), 2); + assert.equal(isValidAttrValue("foo=>bar", false), Validity.enclosed); - assert.equal(isValidAttrValue("foo >", false), 0); + assert.equal(isValidAttrValue("foo >", false), Validity.invalid); - assert.equal(isValidAttrValue("foo > bar", true), 2); + assert.equal(isValidAttrValue("foo > bar", true), Validity.enclosed); - assert.equal(isValidAttrValue("foo, bar", false), 0); + assert.equal(isValidAttrValue("foo, bar", false), Validity.invalid); - assert.equal(isValidAttrValue("foo;", false), 2); + assert.equal(isValidAttrValue("foo;", false), Validity.enclosed); - assert.equal(isValidAttrValue("foo;", true), 0); + assert.equal(isValidAttrValue("foo;", true), Validity.invalid); - assert.equal(isValidAttrValue("foo --", false), 2); + assert.equal(isValidAttrValue("foo --", false), Validity.enclosed); - assert.equal(isValidAttrValue("foo --", true), 0); + assert.equal(isValidAttrValue("foo --", true), Validity.invalid); - assert.equal(isValidAttrValue("foo bar", false), 0); + assert.equal(isValidAttrValue("foo bar", false), Validity.invalid); - assert.equal(isValidAttrValue("a &&\nb", true), 1); + assert.equal(isValidAttrValue("a &&\nb", true), Validity.valid); - assert.equal(isValidAttrValue("a && (\nb\n)", true), 2); + assert.equal(isValidAttrValue("a && (\nb\n)", true), Validity.enclosed);
No description provided.