Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disallow YAML comments in API History #79

Closed
piotrpdev opened this issue Aug 4, 2024 · 1 comment · Fixed by #81
Closed

Disallow YAML comments in API History #79

piotrpdev opened this issue Aug 4, 2024 · 1 comment · Fixed by #81
Labels

Comments

@piotrpdev
Copy link
Collaborator

piotrpdev commented Aug 4, 2024

As discussed in a Slack call and mentioned in https://github.com/piotrpdev/electron/commit/06cc0196c8a5970cd1ee3faade3c8fe49d745850, comments shouldn't be allowed in API History YAML.

Therefore, adding a check for them in the linter would be useful. You can probably just use a RegExp.

Most of them are already caught by checkStrings:

const possibleStringRegex = /^[ \S]+?: *?(\S[ \S]+?)$/gm;

// Special chars in YAML strings may break the parser if not surrounded by quotes,
// including just causing the parser to read a value as null instead of throwing an error
// <https://stackoverflow.com/questions/19109912/yaml-do-i-need-quotes-for-strings-in-yaml>
if (checkStrings) {
const possibleStrings = codeBlock.value.matchAll(possibleStringRegex);
for (const [matchedLine, matchedGroup] of possibleStrings) {
const trimmedMatchedGroup = matchedGroup.trim();
const isMatchedGroupInsideQuotes =
(trimmedMatchedGroup.startsWith('"') && trimmedMatchedGroup.endsWith('"')) ||
(trimmedMatchedGroup.startsWith("'") && trimmedMatchedGroup.endsWith("'"));
// Most special characters won't cause a problem if they're inside quotes
if (isMatchedGroupInsideQuotes) continue;
// I've only seen errors occur when the first or last character is a special character - @piotrpdev
const isFirstCharNonAlphaNumeric =
trimmedMatchedGroup[0].match(nonAlphaNumericDotRegex) !== null;
const isLastCharNonAlphaNumeric =
trimmedMatchedGroup.at(-1)?.match(nonAlphaNumericDotRegex) !== null;
if (isFirstCharNonAlphaNumeric || isLastCharNonAlphaNumeric) {
console.warn(
'Warning occurred while parsing Markdown document:\n\n' +
`'${filepath}'\n\n` +
'Possible string value starts/ends with a non-alphanumeric character.\n\n' +
'This might cause issues when parsing the YAML (might not throw an error)\n\n' +
'Matched group:\n\n' +
`${matchedGroup}\n\n` +
'Matched line:\n\n' +
`${matchedLine}\n\n` +
'API history block:\n\n' +
`${possibleHistoryBlock.value}\n`,
);
// Not throwing an error because it might be a false positive or desired behavior
warningCounter++;
}
}
}

But a line comment is not caught:

<!--
```YAML history
added:
  # This comment is not caught
  - pr-url: https://github.com/electron/electron/pull/22533
```
-->
@piotrpdev piotrpdev added the ✨ enhancement New feature or request label Aug 4, 2024
piotrpdev added a commit that referenced this issue Aug 7, 2024
@piotrpdev piotrpdev added javascript Pull requests that update Javascript code and removed javascript Pull requests that update Javascript code labels Aug 7, 2024
dsanders11 added a commit that referenced this issue Aug 9, 2024
* feat(api-history): `--check-descriptions`

Reference: #78 (comment)

* test(api-history): description test

* fix(api-history): only double quotes

* feat(api-history): `--disallow-comments`

Reference: #79

* feat(api-history): traverse tree instead

* fix(api-history): description `for` loop

Co-authored-by: David Sanders <[email protected]>
Reference: #80 (comment)

* style(api-history): fix lint, leave comment

---------

Co-authored-by: David Sanders <[email protected]>
Copy link

🎉 This issue has been resolved in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants