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

Enable unicode regexp flag #278

Merged
merged 1 commit into from
Nov 25, 2022
Merged

Enable unicode regexp flag #278

merged 1 commit into from
Nov 25, 2022

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Nov 25, 2022

I was looking over the codebase to see how easy it might be to implement a feature, and noticed you're not using the u flag and had some regexps that it helps with ({name}), and figured it'd make a good first contribution :)

(I won't be offended if you don't want to land this either)

Comment on lines +43 to +45
contents.includes(content.replace(/"/gu, '\\"')) ||
contents.includes(content.replace(/'/gu, "\\'"));
Copy link

Choose a reason for hiding this comment

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

why does the u matter here?

lib/generator.ts Outdated
@@ -165,7 +165,7 @@ export async function generate(path: string, options?: GenerateOptions) {
// Update rule doc for each rule.
let initializedRuleDoc = false;
for (const { name, description, schema } of details) {
const pathToDoc = join(path, pathRuleDoc).replace(/{name}/g, name);
const pathToDoc = join(path, pathRuleDoc).replace(/\{name\}/gu, name);
Copy link

Choose a reason for hiding this comment

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

similarly, why does the u flag help here? none of the chars in {name} can be part of a surrogate pair.

Copy link
Owner

Choose a reason for hiding this comment

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

I assume it's just for consistency so require-unicode-regexp can be enabled?

@@ -42,7 +42,7 @@ export function findSectionHeader(
str: string
): string | undefined {
// Get all the matching strings.
const regexp = new RegExp(`## .*${str}.*\n`, 'gi');
const regexp = new RegExp(`## .*${str}.*\n`, 'giu');
Copy link

Choose a reason for hiding this comment

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

this may be a breaking change

Copy link
Owner

Choose a reason for hiding this comment

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

That's a fair concern but realistically I don't think it's an issue and certainly not a blocker. This code is mostly just used for searching for sections like ## Rules, ## Options, ## Config where the flag is unlikely to have an effect. Also I'm still open to breaking changes although I plan to release v1.0.0 very soon.

@@ -227,7 +227,7 @@ function ruleNamesToList(
: ruleName;
return `[\`${ruleNameWithoutPluginPrefix}\`](${
urlRuleDoc
? urlRuleDoc.replace(/{name}/g, ruleName)
? urlRuleDoc.replace(/\{name\}/gu, ruleName)
Copy link

Choose a reason for hiding this comment

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

same question here

@@ -29,7 +29,7 @@ import { hasOptions } from './rule-options.js';

// Example: theWeatherIsNice => The Weather Is Nice
function camelCaseStringToTitle(str: string) {
const text = str.replace(/([A-Z])/g, ' $1');
const text = str.replace(/([A-Z])/gu, ' $1');
Copy link

Choose a reason for hiding this comment

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

and here

lib/rule-list.ts Outdated
@@ -136,7 +136,7 @@ function buildRuleRow(
? EMOJI_HAS_SUGGESTIONS
: '',
[COLUMN_TYPE.NAME]: `[${rule.name}](${(urlRuleDoc ?? pathRuleDoc).replace(
/{name}/g,
/\{name\}/gu,
Copy link

Choose a reason for hiding this comment

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

and here

@bmish bmish added bug Something isn't working internal and removed bug Something isn't working labels Nov 25, 2022
@bmish bmish changed the title chore: enable unicode regexp flag Enable unicode regexp flag Nov 25, 2022
Copy link
Owner

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Happy to accept this but I'll give you a chance to respond to the comments first.

@@ -40,8 +40,8 @@
// in case escaping is needed where the content is referenced.
const hasContent =
contents.includes(content) ||
contents.includes(content.replace(/"/g, '\\"')) ||
contents.includes(content.replace(/'/g, "\\'"));
contents.includes(content.replace(/"/gu, '\\"')) ||

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding

This does not escape backslash characters in the input.
contents.includes(content.replace(/"/g, '\\"')) ||
contents.includes(content.replace(/'/g, "\\'"));
contents.includes(content.replace(/"/gu, '\\"')) ||
contents.includes(content.replace(/'/gu, "\\'"));

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding

This does not escape backslash characters in the input.
@G-Rath
Copy link
Contributor Author

G-Rath commented Nov 25, 2022

@bmish awesome - you've already responded with what I would have said 🙂

I'll resolve the conflicts, but what do you want to do about the CodeQL findings? (I'm pretty sure they're not introduced by this change, it's just that its checking those lines for the first time)

@bmish
Copy link
Owner

bmish commented Nov 25, 2022

@G-Rath I think those CodeQL warnings are the same ones I ignored in the past, now reappearing, so I'll ignore them again. I'll merge after you rebase, thanks!

@bmish bmish merged commit 88fa762 into bmish:main Nov 25, 2022
@G-Rath G-Rath deleted the use-unicode-flag branch November 25, 2022 18:49
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 this pull request may close these issues.

None yet

3 participants