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

fix(Parser): remove redundant nested repeat operator and escape bracket #90

Closed
wants to merge 1 commit into from

Conversation

hendrixfan
Copy link

lib/device_detector/parser.rb:88: warning: regular expression has ']' without escape
lib/device_detector/parser.rb:88: warning: nested repeat operator '+' and '?' was replaced with '*' in regular expression
  • I removed the '?' operators where possible and exchanged them for '*'
  • closes Ruby warnings #89

- with ruby 2.7.3 I get the following warnings(described in podigee#89):

```
lib/device_detector/parser.rb:88: warning: regular expression has ']' without escape
lib/device_detector/parser.rb:88: warning: nested repeat operator '+' and '?' was replaced with '*' in regular expression
```
- I removed the '?' operators where possible and exchanged them for '*'
- closes podigee#89
@hendrixfan
Copy link
Author

oh, I just read your disclaimer

Still, our goal is to use the original, unchanged regex yaml files, in order to mutually benefit from updates and pull request to both the original and the ported versions.

you are not maintaining these Files yourself

@kwent
Copy link

kwent commented Nov 20, 2021

@hendrixfan I think your search and replace replaced stuff you didn't want to modify.

Screen Shot 2021-11-19 at 9 25 09 PM

@hendrixfan
Copy link
Author

@kwent sorry for not elaborating on my changes further.
I meant those changes in the Screenshot. Those Bracket Ranges do have redudant Characters: 5033[ADXJEFGMOTXYQS] X in that case.
I got Ruby warnings about those

@kwent
Copy link

kwent commented Nov 23, 2021

@podigee what is the plan here ? I don't think upstream repositories will ever update those cause it's language specific. Maybe a middleman script to do the conversation and update one every few months would make that workflow easier.

@yakschuss
Copy link

Silence on the airwaves 😅

@aried3r
Copy link
Contributor

aried3r commented Dec 7, 2021

I don't think upstream repositories will ever update those cause it's language specific.

Are they, though? I believe this change should work the same in both languages. There's only a few occurrences (of the initial issue, not the duplicate characters) that I could find, so perhaps they'll accept a PR?

Examples:
https://github.com/matomo-org/device-detector/blob/ebd8a07e4b69088c0e34f29ec72dc162c34c9264/regexes/device/mobiles.yml#L23244
https://github.com/matomo-org/device-detector/blob/ebd8a07e4b69088c0e34f29ec72dc162c34c9264/regexes/device/mobiles.yml#L23253
https://github.com/matomo-org/device-detector/blob/ebd8a07e4b69088c0e34f29ec72dc162c34c9264/regexes/client/browsers.yml#L2370

@Spone
Copy link

Spone commented Jan 5, 2022

I guess we should try to open an issue / PR against https://github.com/matomo-org/device-detector and discuss the changes with their maintainers. If the changes do not break other languages, there may be a chance to get it merged?

@nickcampbell18
Copy link

@Spone I have raised that ticket here, if anybody wants to +1 it for visibility: matomo-org/device-detector#6990

@sanchezzzhak
Copy link

Regular expressions have been fixed, but you still need to implement the changes that we made here for OS
https://github.com/matomo-org/device-detector/releases/tag/5.0.0

We now have a validator that is guaranteed not to let bad regular expressions slip through.
Thank you all for participating.

@hendrixfan hendrixfan closed this Feb 17, 2022
@marcelgo
Copy link

Thanks everybody. We will provide a new version of the gem soon.

@subelsky
Copy link

in case anyone lands here, v1.0.7 of the gem fixes the issue. thanks @marcelgo @hendrixfan

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

Successfully merging this pull request may close these issues.

Ruby warnings
9 participants