Add parseDotInt#24
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the workflow documentation in AGENTS.md and introduces a new utility function, parseDotInt, to handle integer parsing for Graphviz attributes. In src/viz.ts, the linelength attribute parsing was updated to use this new utility. Feedback identifies that the parseDotInt regex incorrectly allows decimal values, which contradicts its intended use for strict integers. Furthermore, a regression was noted in src/viz.ts where the validation check was weakened from Number.isInteger to Number.isNaN, potentially allowing invalid non-integer values to be accepted.
There was a problem hiding this comment.
Pull request overview
Adds a new utility helper (parseDotInt) and switches Viz’s linelength attribute parsing to use it, aiming to validate numeric DOT attribute values more strictly/consistently.
Changes:
- Import and use
parseDotIntwhen parsinglinelengthinsrc/viz.ts. - Add
parseDotInt(with a numeric regex gate) tosrc/utils.ts. - Strengthen workflow instructions in
AGENTS.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/viz.ts |
Uses parseDotInt for linelength parsing and adjusts validation logic. |
src/utils.ts |
Adds parseDotInt and a regex intended to constrain numeric parsing. |
AGENTS.md |
Adds a “mandatory workflow” emphasis note to agent guidelines. |
Comments suppressed due to low confidence (1)
src/viz.ts:265
- The new
linelengthvalidation no longer enforces that the parsed value is an integer:isMaxLineLengthonly range-checks, so values like60.5will pass and be forwarded to the backend even though the error message (and existing behavior) sayslinelengthmust be an integer. Keep an explicit integer check here (or makeparseDotIntreject non-integer forms such as60.0/60.5) before accepting the value.
const number = NormalizedAttributes.isText(linelengthValue)
? parseDotInt(linelengthValue.text)
: Number.NaN;
if (Number.isNaN(number) || !isMaxLineLength(number)) {
return failureResult([
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the workflow documentation in AGENTS.md to emphasize that all steps are mandatory and introduces a parseDotNumber utility in src/utils.ts for parsing Graphviz numeric attributes. The utility is integrated into src/viz.ts to handle the linelength attribute. Feedback was provided regarding the parseDotNumber regex, which currently fails to account for valid DOT numerals with leading or trailing decimal points, along with a suggestion to align the implementation with the Graphviz specification.
| const DOT_NUMBER_RE = /^-?\d+(\.\d+)?$/; | ||
| export function parseDotNumber(value: string): number { | ||
| return DOT_NUMBER_RE.test(value) ? Number(value) : Number.NaN; | ||
| } |
There was a problem hiding this comment.
The implementation name parseDotNumber and its support for decimal points in the regex contradict the pull request title Add parseDotInt.
Furthermore, the regex /^-?\d+(\.\d+)?$/ does not account for all valid DOT numerals. According to the Graphviz specification, a numeral can have a leading or trailing decimal point (e.g., .5 or 5.).
If this function is intended to be a general-purpose DOT numeral parser, I recommend updating the regex to be spec-compliant. If it is specifically for integers, it should be renamed to parseDotInt and the regex simplified to /^-?\d+$/.
| const DOT_NUMBER_RE = /^-?\d+(\.\d+)?$/; | |
| export function parseDotNumber(value: string): number { | |
| return DOT_NUMBER_RE.test(value) ? Number(value) : Number.NaN; | |
| } | |
| const DOT_NUMBER_RE = /^-?(?:\d+(?:\.\d*)?|\.\d+)$/; | |
| export function parseDotNumber(value: string): number { | |
| return DOT_NUMBER_RE.test(value) ? Number(value) : Number.NaN; | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the workflow documentation to emphasize mandatory steps and introduces a parseDotNumber utility for more robust parsing of Graphviz attributes. A critical logic error was identified in the regular expression used for number parsing, which could allow partial matches and invalid strings to be accepted; a fix was suggested to correctly group the alternation and support trailing decimal points.
| .slice(1, -1); | ||
| } | ||
|
|
||
| const DOT_NUMBER_RE = /^-?\d+(\.\d+)?|-?\.\d+$/; |
There was a problem hiding this comment.
The DOT_NUMBER_RE regular expression has a logic error because the alternation (|) is not grouped, causing the anchors (^ and $) to only apply to one side of the OR. This allows partial matches (e.g., "123abc" or "abc.123") to pass the test. Additionally, to align with the project's parser and the Graphviz specification, it should support trailing decimal points like 123.. Note that while the PR title mentions parseDotInt, this implementation correctly provides a more general parseDotNumber utility.
| const DOT_NUMBER_RE = /^-?\d+(\.\d+)?|-?\.\d+$/; | |
| const DOT_NUMBER_RE = /^-?(\.\d+|\d+(\.\d*)?)$/; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/utils.ts:12
- The PR title suggests adding
parseDotInt, but this change introducesparseDotNumberand uses it forlinelength(an integer-only attribute). If the intent is specifically integer parsing/validation, consider renaming toparseDotInt(or adding a dedicatedparseDotIntwrapper) to match the stated goal and make call sites clearer.
const DOT_NUMBER_RE = /^-?\d+(\.\d+)?|-?\.\d+$/;
export function parseDotNumber(value: string): number {
return DOT_NUMBER_RE.test(value) ? Number(value) : Number.NaN;
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the workflow documentation in AGENTS.md, introduces a parseDotNumber utility for DOT-specific number parsing, and applies it to linelength validation in Viz. It also moves ESLint directives within parser.ts and expands test coverage for invalid linelength values. Review feedback identifies that the new regex for number parsing fails to account for valid trailing decimal points and suggests that moving the ESLint directives into individual methods creates redundancy and inconsistency.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/parser.ts:171
- Same as above: this
/* eslint-disable unicorn/prefer-code-point */directive is not scoped to the method and will keep the rule disabled beyond this point unless explicitly re-enabled. Prefereslint-disable-next-lineon thecharCodeAtstatement or re-enable immediately after.
#peekChar(): number {
// To make parser internally consistent, all characters are read as UTF-16:
/* eslint-disable unicorn/prefer-code-point */
return this.#dotStr.charCodeAt(this.#nextIndex);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the workflow guidelines in AGENTS.md, refactors ESLint disable comments in src/parser.ts, and introduces a parseDotNumber utility to safely parse numeric values from DOT attributes, accompanied by expanded test coverage for invalid linelength values. The reviewer feedback suggests keeping the file-level ESLint disable comment to avoid repetitive method-level disables, and moving parseDotNumber to src/parser.ts to prevent a circular dependency between src/parser.ts and src/utils.ts.
|
|
||
| // To make parser internally consistent, all characters are read as UTF-16: | ||
| /* eslint-disable unicorn/prefer-code-point */ | ||
|
|
||
| const Char = { |
There was a problem hiding this comment.
Restoring the file-level eslint-disable comment is cleaner and avoids ESLint errors in other methods like #peekAheadChar which also use charCodeAt but were missed in this PR.
| // To make parser internally consistent, all characters are read as UTF-16: | |
| /* eslint-disable unicorn/prefer-code-point */ | |
| const Char = { | |
| // To make parser internally consistent, all characters are read as UTF-16: | |
| /* eslint-disable unicorn/prefer-code-point */ | |
| const Char = { |
| #readChar(): number { | ||
| // To make parser internally consistent, all characters are read as UTF-16: | ||
| /* eslint-disable unicorn/prefer-code-point */ | ||
| return this.#dotStr.charCodeAt(this.#nextIndex++); | ||
| } | ||
|
|
||
| #peekChar(): number { | ||
| // To make parser internally consistent, all characters are read as UTF-16: | ||
| /* eslint-disable unicorn/prefer-code-point */ | ||
| return this.#dotStr.charCodeAt(this.#nextIndex); | ||
| } |
| export function isNumberToken(str: string): boolean { | ||
| const lexer = new Lexer(str); | ||
| const token = lexer.nextToken(); | ||
| return token.kind === Kind.Number && token.length === str.length; | ||
| } |
There was a problem hiding this comment.
Moving parseDotNumber to src/parser.ts next to isNumberToken avoids a circular dependency between src/parser.ts and src/utils.ts.
| export function isNumberToken(str: string): boolean { | |
| const lexer = new Lexer(str); | |
| const token = lexer.nextToken(); | |
| return token.kind === Kind.Number && token.length === str.length; | |
| } | |
| export function isNumberToken(str: string): boolean { | |
| const lexer = new Lexer(str); | |
| const token = lexer.nextToken(); | |
| return token.kind === Kind.Number && token.length === str.length; | |
| } | |
| export function parseDotNumber(value: NormalizedAttributeValue): number { | |
| return NormalizedAttributes.isText(value) && isNumberToken(value.text) | |
| ? Number.parseFloat(value.text) | |
| : Number.NaN; | |
| } |
| import { | ||
| NormalizedAttributes, | ||
| type NormalizedAttributeValue, | ||
| } from './normalize-graph.ts'; | ||
| import { isNumberToken } from './parser.ts'; |
| export function parseDotNumber(value: NormalizedAttributeValue): number { | ||
| return NormalizedAttributes.isText(value) && isNumberToken(value.text) | ||
| ? Number.parseFloat(value.text) | ||
| : Number.NaN; | ||
| } |
| import { parseDot } from './parser.ts'; | ||
| import { formatValueForDiagnostics } from './utils.ts'; | ||
| import { formatValueForDiagnostics, parseDotNumber } from './utils.ts'; |
There was a problem hiding this comment.
Import parseDotNumber from ./parser.ts instead of ./utils.ts to align with the move and avoid circular dependencies.
| import { parseDot } from './parser.ts'; | |
| import { formatValueForDiagnostics } from './utils.ts'; | |
| import { formatValueForDiagnostics, parseDotNumber } from './utils.ts'; | |
| import { parseDot, parseDotNumber } from './parser.ts'; | |
| import { formatValueForDiagnostics } from './utils.ts'; |
|
|
||
| #readChar(): number { | ||
| // To make parser internally consistent, all characters are read as UTF-16: | ||
| /* eslint-disable unicorn/prefer-code-point */ |
| /* eslint-disable unicorn/prefer-code-point */ | ||
| return this.#dotStr.charCodeAt(this.#nextIndex++); | ||
| } | ||
|
|
||
| #peekChar(): number { | ||
| // To make parser internally consistent, all characters are read as UTF-16: | ||
| /* eslint-disable unicorn/prefer-code-point */ |
No description provided.