feat: modifier support by brontybuutveld · Pull Request #50 · fent/ret.js

brontybuutveld · 2026-02-15T06:39:28Z

Adds support for group modifiers see: MDN.

This is an extension to the groups, might not be a good approach since I'm not familiar with the requirements and considerations of this project.

If I've gotten something wrong, and you have the time to explain please let me know what I can change in this implementation to make it better.

Best,
Bronty

fent

Hello, thank you for the pull request! I've looked through the code, the overall idea makes sense. I've added some comments.

I'd also be great to add tests, as that makes it easier to quickly see if things works, catching edge cases, and see if other things break

Sorry for taking so long to look at this v_v

brontybuutveld · 2026-10-23T21:44:50Z

Thank you for taking the time to review my code! I’ve updated the implementation to use a modifier object with ignoreCase, multiline, and dotAll, using true/false/null as you suggested. I also removed the dash property and the wrapper functions.

Modifier groups are now non-capturing, accept only lowercase flags, and support disable-only forms like (?-i:hello). Reconstruction derives the header from the modifier settings, and validation catches repeated flags and malformed headers with errors closer to the JavaScript runtime.

I’ve added tests for parsing, reconstruction, nesting, capture numbering, and invalid syntax, plus comparisons against Node’s regex behavior. The tests also caught an issue caused by decoding character escapes before parsing group headers. For example, (?\u0069:hello) could be treated as (?i:hello), even though JavaScript rejects the escaped flag. Modifier headers now require literal characters, while escapes inside the group body continue to work normally.

All tests are passing locally.

Please let me know if anything else needs addressing!

Best,
Bronty

fent

Thanks for taking the time to address my previous comments. Left a couple of new comments.

Interesting use of creating a source map to keep track of escaped characters. That also surfaces another error in this library relating to decoded strings in objects, like (?\u003aa) which decodes to (?:a) is a js error, but ret will parse it. It's unrelated to the PR, but thank you for surfacing it :)

fent · 2026-10-24T23:26:15Z

+      throw new SyntaxError(`Invalid regular expression: /${regexpStr}/: Invalid flag group`);
+    }
+    if (str[index] !== ':' || regexpStr[sourceMap[index]] !== ':') {
+      throw new SyntaxError(`Invalid regular expression: /${regexpStr}/: Invalid group`);


Could you add the column to all of these errors? Just to keep things consistent with other error messages

Suggested change

throw new SyntaxError(`Invalid regular expression: /${regexpStr}/: Invalid group`);

throw new SyntaxError(`Invalid regular expression: /${regexpStr}/: Invalid group at column ${index});

fent · 2026-10-24T23:28:01Z

+/**
+ * Valid characters for group modifiers.
+ */
+const modeModifiers = /^[ims]$/;


let's just use modifierProperties's keys to match modifiers, gives us one place to maintain instead of two. Something like Object.hasKey(modifierProperties, flag) instead of modeModifiers.test(flag)

brontybuutveld · 2026-10-25T00:59:42Z

Thanks for taking another look! Modifier validation now checks the keys of modifierProperties. All the new modifier errors now include a column. These use the original source positions, so an earlier decoded escape doesn’t shift the reported location.

I’ve added tests for those positions, including escaped prefixes and unfinished headers.

As an aside, I noticed a reconstruction edge case involving dot-all. Ret currently simplifies [^\n\r\u2028\u2029] to ., since they normally match the same characters. Inside (?s:...), though, they differ: the explicit class still excludes line terminators, while . includes them. That means reconstructing (?s:[^\n\r\u2028\u2029]) currently produces (?s:.), changing its meaning.

I’ve left that out of this PR, as preserving the distinction may need a separate discussion about the token representation. I also left the escaped non-capturing-group issue you mentioned outside this change.

Best,
Bronty

fent

Thanks! Added a couple more comments

fent · 2026-10-25T02:39:54Z

+  };
+
+  const tokenizeModifiers = (group: Group, index: number): number => {
+    const modifier: NonNullable<Group['modifier']> = {


I don't think this NonNullable is needed, when I remove it, it still works

Suggested change

const modifier: NonNullable<Group['modifier']> = {

const modifier: Group['modifier'] = {

fent · 2026-10-25T02:44:56Z

  };

+  const modifierErr = (message: string, index: number): never => {
+    const column = sourceMap[index] ?? regexpStr.length;


Ah, nice catch with using the source map for the column. All of the other previously existing error messages could point at the wrong column :/. But again, unrelated to the PR

brontybuutveld · 2026-10-25T08:35:00Z

Oh, by the way the commits are AI generated. My goal here was just to get modifiers working in randexp. Wasn't really expecting anything out of it since this looked like a dormant solo project. I really do appreciate your feedback, and I'm happy to help out, but I understand if not. I don't want to burden you.

fent · 2026-10-25T09:52:00Z

That's fine, as long as it's a good contribution. This was something that ret was missing, so I appreciate that you took the time to share your changes. And now more people that use the library can benefit from it too, so it's not a burden at all.

PR looks good btw, going to merge in a bit. Thanks again for taking the time to contribute!

github-actions · 2026-10-25T18:21:06Z

🎉 This PR is included in version 0.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Reviews the 13 upstream commits that [unabandoned/.github#53](unabandoned/.github#53) lists for this fork. **4 are ported and 9 declined**, each recorded with its reason under `upstream.reviewed` in `.unabandoned.yml`. **After this PR, `lib/` is identical to upstream master.** ## Ported (lib/ applies unchanged) - **[`b38d6691`](fent@b38d6691) fix: reject escaped characters in group syntax (fent#58).** The tokenizer decodes escapes before it parses, so `(?\x3Da)` was read as the lookahead `(?=a)`. Likewise `(?\x21a)`, `(?\x3Aa)` and `(?\x3Cn>a)` became `(?!a)`, `(?:a)` and a named group. JavaScript rejects all of these, and `reconstruct` turned them into different, valid regexps. They now throw, and group errors report the column in the original source. - **[`720c2ff8`](fent@720c2ff8) feat: group modifier support (fent#50).** `(?im-s:a)` is tokenized as a non-capturing group with a `modifier: { ignoreCase, multiline, dotAll }` object, and `reconstruct` writes it back. Its README section is included. b38d669 builds on the source map this commit adds. - **[`7fbd6e02`](fent@7fbd6e02) fix(types).** `Range` leaves the `Token` union for a new `SetToken`, `reconstruct` accepts a `Range`, and `tokenizeClass`'s `regexpStr` defaults to `''`. - **[`c26bea58`](fent@c26bea58) refactor.** Drops an empty-name check that the first-character check already makes unreachable. ## Tests Upstream's tests for these are TypeScript on mocha. Here they are plain JS on node:test, and they run against the built `dist/`: - `test/modifier-test.js`: upstream's `modifier-test.ts`, with only the imports and type annotations removed. - The `strToChars` source-map tests are appended to `util-test.js`. - The escaped-group tests are appended to `error-test.js` and `main-test.js`. `npm test` gives **342 pass, 0 fail**. With `lib/` reverted, **74 fail**. `dist/` is rebuilt with this repo's TypeScript. **Effect on `@unabandoned/randexp`:** its suite passes against this build. Modifier groups that ret used to reject now parse. randexp generates their body but ignores the modifier. ## Declined | Commits | Why | |---|---| | `0609d0d0` `fcc7fae4` | Test-runner migrations, to mocha and then to TypeScript with ts-node and eslint. This fork already runs the vows suite on node:test through `test/vows-adapter.js`. | | `a7207c2b` | `tsconfig.build.json`. Our `tsconfig.json` already sets `rootDir: ./lib`, and the tests are JS. | | `4b603034` `749035a2` | Upstream's lint, nodejs and release workflows, plus badges for `fent/ret.js`. | | `2b53f4ec` `45242341` `2076ce01` `d8fc932f` | dependabot, npm-audit and eslint, typescript-eslint and @types/node bumps on upstream's lockfile. This fork has no lockfile and no eslint, and Renovate manages `@types/node`. d8fc932's tsconfig.json change only strips comments; the options it keeps are already ours. | Typed `feat`, so it releases as a patch (pre-1.0, `bump-patch-for-minor-pre-major`). --- _Generated by [Claude Code](https://claude.ai/code)_

feat: modifier support

6d49705

fent requested changes Oct 23, 2026

View reviewed changes

brontybuutveld added 4 commits October 24, 2026 10:03

Merge branch 'fent:master' into master

9299d2b

fix: align group modifiers with JavaScript syntax

5b3f4fe

fix: reject escaped syntax in modifier headers

b86aa93

docs: explain group modifier tokens and normalization

1109514

fent requested changes Oct 24, 2026

View reviewed changes

brontybuutveld added 2 commits October 25, 2026 13:26

refactor: derive modifier validation from property lookup

5630b3f

fix: report source columns in modifier errors

7f0edf7

fent requested changes Oct 25, 2026

View reviewed changes

fent mentioned this pull request Oct 25, 2026

Some encoded characters in groups should throw an error #55

Closed

refactor: simplify modifier type annotation

0a0e4cd

fent mentioned this pull request Oct 25, 2026

Encoded patterns can show error messages with incorrect columns #56

Open

fent approved these changes Oct 25, 2026

View reviewed changes

fent merged commit 720c2ff into fent:master Oct 25, 2026
6 checks passed

github-actions Bot added the released label Oct 25, 2026

fent mentioned this pull request Oct 27, 2026

perf: improve tokenizer scanning and add benchmark suite #51

Open

danshome mentioned this pull request Oct 28, 2026

fix: reject escaped characters in group syntax #58

Merged

00o-sh mentioned this pull request Nov 8, 2026

feat: parse group modifiers and reject escaped group syntax unabandoned/ret.js#42

Merged

	throw new SyntaxError(`Invalid regular expression: /${regexpStr}/: Invalid group`);
	throw new SyntaxError(`Invalid regular expression: /${regexpStr}/: Invalid group at column ${index});

	const modifier: NonNullable<Group['modifier']> = {
	const modifier: Group['modifier'] = {

Conversation

brontybuutveld commented Feb 15, 2026

Uh oh!

fent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brontybuutveld commented Oct 23, 2026

Uh oh!

fent left a comment

Choose a reason for hiding this comment

Uh oh!

fent Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

fent Oct 24, 2026

Choose a reason for hiding this comment

Uh oh!

brontybuutveld commented Oct 25, 2026

Uh oh!

fent left a comment

Choose a reason for hiding this comment

Uh oh!

fent Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

fent Oct 25, 2026

Choose a reason for hiding this comment

Uh oh!

brontybuutveld commented Oct 25, 2026

Uh oh!

fent commented Oct 25, 2026

Uh oh!

Uh oh!

github-actions Bot commented Oct 25, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants