Skip to content

Comments

Fix incorrect regex on Person.name field#1089

Merged
rodaine merged 3 commits intobufbuild:mainfrom
mattnworb:patch-1
Mar 25, 2024
Merged

Fix incorrect regex on Person.name field#1089
rodaine merged 3 commits intobufbuild:mainfrom
mattnworb:patch-1

Conversation

@mattnworb
Copy link
Contributor

The example regular expression introduced in 10eb9db does not seem to work as intended, due to nesting a negated range inside another range:

❯ jshell --class-path ~/.m2/repository/com/google/re2j/re2j/1.7/re2j-1.7.jar
|  Welcome to JShell -- Version 17.0.9
|  For an introduction type: /help intro

jshell> com.google.re2j.Pattern p = com.google.re2j.Pattern.compile("^[^[0-9]A-Za-z]+( [^[0-9]A-Za-z]+)*$");
p ==> ^[^[0-9]A-Za-z]+( [^[0-9]A-Za-z]+)*$

jshell> p.matches(pattern, "Protocol Buffers");
$4 ==> false

jshell> p.matches(pattern, "Matt Brown");
$5 ==> false

The nesting of ranges in the example is also redundant - if you are going to specify that only [A-Za-z] is valid than including [^0-9]` in that range is not necessary.

This commit fixes the example regular expression to match the example "name" mentioned later in the README.

The example regular expression introduced in bufbuild@10eb9db does not seem to work as intended, due to nesting a negated range inside another range:

```
❯ jshell --class-path ~/.m2/repository/com/google/re2j/re2j/1.7/re2j-1.7.jar
|  Welcome to JShell -- Version 17.0.9
|  For an introduction type: /help intro

jshell> com.google.re2j.Pattern p = com.google.re2j.Pattern.compile("^[^[0-9]A-Za-z]+( [^[0-9]A-Za-z]+)*$");
p ==> ^[^[0-9]A-Za-z]+( [^[0-9]A-Za-z]+)*$

jshell> p.matches(pattern, "Protocol Buffers");
$4 ==> false

jshell> p.matches(pattern, "Matt Brown");
$5 ==> false
```

The nesting of ranges in the example is also redundant - if you are going to specify that only `[A-Za-z]` is valid than including [^0-9]` in that range is not necessary.

This commit fixes the example regular expression to match the example "name" mentioned later in the README.
@CLAassistant
Copy link

CLAassistant commented Mar 25, 2024

CLA assistant check
All committers have signed the CLA.

@rodaine rodaine changed the title readme: fix incorrect regex on Person.name field Fix incorrect regex on Person.name field Mar 25, 2024
@rodaine rodaine added the Documentation Documentation related label Mar 25, 2024
@rodaine
Copy link
Member

rodaine commented Mar 25, 2024

Hey, @mattnworb, and thanks for the patch!

Hm, looks like at some point it was made inconsistent with the regex that was originally in the readme: '^[^\d\s]+( [^\d\s]+)*$'. I'm guessing to avoid having to show the gnarly escaping that would have required. Can you make line 61 consistent with your change as well?

@mattnworb
Copy link
Contributor Author

@rodaine ah good catch, I believe the inconsistency with line 61 was introduced in 10eb9db. I'll push a commit to make them consistent

@mattnworb
Copy link
Contributor Author

noticed an occurrence of the regex used in python/README.md too

@rodaine
Copy link
Member

rodaine commented Mar 25, 2024

Looks good! Once you sign the CLA we can get this merged 😁

@mattnworb
Copy link
Contributor Author

done, thanks!

Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

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

Thanks again! 😁

@rodaine rodaine enabled auto-merge (squash) March 25, 2024 19:41
@rodaine rodaine merged commit 1157c05 into bufbuild:main Mar 25, 2024
@mattnworb mattnworb deleted the patch-1 branch March 25, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Documentation Documentation related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants