observablehq / feedback

Customer submitted bugs and feature requests
42 stars 3 forks source link

Capturing group in Find and Replace stick to the very first match while it shouldn't #480

Closed galopin closed 1 month ago

galopin commented 1 year ago

Describe the bug

Modifying a group of cells using the Find and Replace regular expression mode may not yield the expected result.

For example, trying to replace all day index values from 3 to 11 using the regular expression ((?:[-/]|, )0?9(?:[-/]|, ))0?3 and the substitution string $111 in the following cells

false

new Date("2022-09-03") + "" === new Date("2022/09/03")

false

date("2022-09-03") + "" === date(2022, 9, 3) + ""

yields

true

new Date("2022-09-11") + "" === new Date("2022-09-11") + ""

SyntaxError: Invalid number

new Date("2022-09-11") + "" === new Date(2022-09-11) + ""

First capturing group ((?:[-/]|, )0?9(?:[-/]|, )) value stick to the very first match and is not reset in between :bug:

To Reproduce

Steps to reproduce the behavior:

  1. In the field with the magnifying glass, enter this regular expression: ((?:[-/]|, )0?9(?:[-/]|, ))0?3
  2. In the field with the opposing arrow, enter this substitution string: $111
  3. Click the "Regular expression" check box
  4. Hit the replace-all-instances icon

Expected behavior

It should yield instead

false

new Date("2022-09-11") + "" === new Date("2022/09/11") + ""

false

new Date("2022-09-11") + "" === new Date(2022, 9, 11) + ""

See Find and Replace expected output on regex101…

galopin commented 1 year ago

I wonder why the issue I reported is not receiving any attention ⁉️ This is kind of baffling 👎

CobusT commented 1 year ago

Sorry for letting this slip through the cracks. We normally triage these much quicker to determine whether it is a bug or feature request. This does look like a bug at the moment and we will take a closer look.

mootari commented 1 year ago

Alternative repro:

  1. Create a cell containing "AA", "BB"
  2. Open search/replace, enable regex
  3. Enter (AA|BB)\b as the search term
  4. Enter $1C as the replacement
  5. Replace all matches

Expected result: "AAC", "BBC" Actual result: "AAC", "AAC"

galopin commented 1 year ago

It looks like someone has already flagged this issue.

mootari commented 1 month ago

Just ran into this myself and had to revert a set of ~30 changes.

If the fix is non-trivial then we should at least consider disabling the "Replace all" button while "Regular expression" is checked, since the workaround is to replace matches one by one.

galopin commented 1 month ago

This is an annoying and lingering issue, to say the least.

galopin commented 1 month ago

@mootari Kudos for fixing this annoying issue. It's working as expected now :smile:

However, I wonder if your hotfix introduced another side effect with empty capturing groups!?

For example, applying the regular expression floor\((?:(?!\()([^\/]+?)|\(([^()]+)\))\s+\/\s+(.+)\) and the substitution string q($1$2, $3) to the following cells

z = 19930

z = floor(+new Date() / 1000 / meanSolarDay) // today

mp = 4

mp = floor((5 * dayOfYear + 2) / offset) // mp === 0 is March, mp === 11 is February

yields respectively

SyntaxError: Unexpected token

z = q(+new Date()undefined, 1000 / meanSolarDay) // today

mp = RuntimeError: undefined5 is not defined

mp = q(undefined5 * dayOfYear + 2, offset) // mp === 0 is March, mp === 11 is February

IMHO, an empty capturing group should yield an empty string instead of inserting undefined :confused:

Expected output should be equivalent to the replace() method of String values:

"z = q(+new Date(), 1000 / meanSolarDay)"

`z = floor(+new Date() / 1000 / meanSolarDay)`.replace(
  /floor\((?:(?!\()([^\/]+?)|\(([^()]+)\))\s+\/\s+(.+)\)/,
  "q($1$2, $3)"
)

"mp = q(5 * dayOfYear + 2, offset)"

`mp = floor((5 * dayOfYear + 2) / offset)`.replace(
  /floor\((?:(?!\()([^\/]+?)|\(([^()]+)\))\s+\/\s+(.+)\)/,
  "q($1$2, $3)"
)

Please see Find and Replace expected output on regex101…

mootari commented 1 month ago

@galopin Great catch, although that's unfortunately a bug that already existed before my fix. 😅

Could I bother you to file a new issue so that we can track it? For the repro it should suffice to mention A as subject, A(B)? as search and A$1 as replace.

mootari commented 1 month ago

The "undefined" replacement for optional capture groups should be fixed now as well.