Skip to content

Support default values for non-optional properties (name=value syntax)#187

Closed
Copilot wants to merge 3 commits intomainfrom
copilot/add-support-default-values
Closed

Support default values for non-optional properties (name=value syntax)#187
Copilot wants to merge 3 commits intomainfrom
copilot/add-support-default-values

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 25, 2026

  • Modify src/parser/tokenizers/name.ts to handle name=value (non-optional with default value)
  • Align non-optional path with optional: use split/join/trim and add spec:name:invalid-default validation
  • Add unit tests in tests/unit/spec-name-tokenizer.spec.ts for the new behavior (including param=value=value invalid case)
  • Add e2e test tests/e2e/issue-186.spec.js for the JSDoc property example
  • All 189 tests pass
Original prompt

The issue #186 requests support to treat default values for non-optional properties. As discussed, when documenting with JSDoc such as:

/** Some annoying set of bitmasks or something.
 *
 * @property {number} BITMASK_VALUE_A=16 - blah blah
 * @property {number} BITMASK_VALUE_B=32 - the other thing
 */

These should be interpreted so that BITMASK_VALUE_A has a default of 16 and BITMASK_VALUE_B has a default of 32. This handling is necessary to accommodate practices for defining constant values. Implement a fix with these requirements:

  1. Update the parser logic to correctly recognize =value in property declarations as defaults.
  2. Ensure existing functionality, such as optional properties, retain their expected behavior.
  3. Include tests to verify various scenarios for default values with properties in the parser.

This adjustment will ensure support for such syntax which has a precedent as detailed in related issues (e.g., gajus/eslint-plugin-jsdoc#1675).

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: Propose a PR for this issue

Assistant: A pull request has been proposed to address the issue by implementing support for default values in non-optional properties.

This pull request was created from Copilot chat.


⌨️ Start Copilot coding agent tasks without leaving your editor — available in VS Code, Visual Studio, JetBrains IDEs and Eclipse.

Copilot AI changed the title [WIP] Implement support for default values in non-optional properties Support default values for non-optional properties (name=value syntax) Mar 25, 2026
Copilot AI requested a review from syavorsky March 25, 2026 23:49
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 25, 2026

⚠️ Missing Changeset

This PR doesn't include a changeset file. If this PR contains changes that should be noted in the changelog, please add a changeset by running:

npm run release:add

This will prompt you to describe the changes and select the version bump type.


💡 If this PR doesn't require a changeset (e.g., docs, tests, or internal changes), add the skip-changeset label to this PR.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for parsing default values from unbracketed JSDoc @property names using name=value syntax, aligning with common “constant bitmask” documentation patterns (issue #186).

Changes:

  • Extend nameTokenizer to split unbracketed name=value into spec.name + spec.default.
  • Add unit tests covering non-optional defaults (string, numeric, and empty-default error).
  • Add an e2e regression test for the issue’s canonical @property BITMASK_VALUE_A=16 use case.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/parser/tokenizers/name.ts Adds non-optional name=value parsing to populate spec.default.
tests/unit/spec-name-tokenizer.spec.ts Adds unit coverage for unbracketed defaults and empty-default error handling.
tests/e2e/issue-186.spec.js Validates end-to-end parsing output for @property constants with defaults.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +114 to +139
} else {
const eqIndex = name.indexOf('=');
if (eqIndex !== -1) {
defaultValue = name.slice(eqIndex + 1);
name = name.slice(0, eqIndex);

if (name === '') {
spec.problems.push({
code: 'spec:name:empty-name',
message: 'empty name',
line: spec.source[0].number,
critical: true,
});
return spec;
}

if (defaultValue === '') {
spec.problems.push({
code: 'spec:name:empty-default',
message: 'empty default value',
line: spec.source[0].number,
critical: true,
});
return spec;
}
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-optional default parsing diverges from the optional [name=value] path: it doesn’t trim around = and it skips the spec:name:invalid-default validation (e.g. name=value=value would now be accepted unbracketed but rejected when bracketed). Consider reusing the same split/join/trim + invalid-default check logic for both branches so defaults behave consistently.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in 786bf5f — the non-optional path now uses the same split/join/trim approach and spec:name:invalid-default check as the optional path.

Comment on lines +623 to +691
test('non-optional with default', () => {
expect(
tokenize(
seedSpec({
source: [
{
number: 1,
source: '...',
tokens: seedTokens({
description: 'param=value param description',
}),
},
],
})
)
).toEqual(
seedSpec({
name: 'param',
optional: false,
default: 'value',
source: [
{
number: 1,
source: '...',
tokens: seedTokens({
name: 'param=value',
postName: ' ',
description: 'param description',
}),
},
],
})
);
});

test('non-optional with numeric default', () => {
expect(
tokenize(
seedSpec({
source: [
{
number: 1,
source: '...',
tokens: seedTokens({
description: 'BITMASK_VALUE_A=16 blah blah',
}),
},
],
})
)
).toEqual(
seedSpec({
name: 'BITMASK_VALUE_A',
optional: false,
default: '16',
source: [
{
number: 1,
source: '...',
tokens: seedTokens({
name: 'BITMASK_VALUE_A=16',
postName: ' ',
description: 'blah blah',
}),
},
],
})
);
});
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new non-optional name=value behavior, it would be good to add a unit test covering the same invalid-default edge case that exists for optional names (e.g. param=value=value should surface spec:name:invalid-default if you align validation across both paths). This helps ensure the bracketed and unbracketed default syntaxes don’t drift over time.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot apply changes based on this feedback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 786bf5f. The non-optional name=value path now uses the same split/join/trim approach as the optional path and includes the spec:name:invalid-default validation (rejecting param=value=value). A matching unit test was also added.

…id-default check, test)

Agent-Logs-Url: https://github.com/syavorsky/comment-parser/sessions/5da727bd-4934-4851-b3ad-eea43977b1a2

Co-authored-by: syavorsky <455779+syavorsky@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants