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

[enh] Add PCRE regex support #102

Merged
merged 4 commits into from Dec 13, 2018

Conversation

Projects
None yet
5 participants
@zamentur
Copy link
Contributor

zamentur commented Jun 24, 2018

As the original branch of this #44 is missing, I have create a new one.

The problem

Lua pattern are not well known and a lot of packager don't know how write this kind of regex. PCRE seems more familiar.

Solution

Add support of PCRE, and keep support of lua pattern during a transition period.

PR Status

Tested with nextcloud

How to test

./ynh-dev use-git ssowat
setup one of this app and check it works as expected in their install script:

  • kodi,
  • nextcloud (feature using /%.well%-known/)
  • lutim as private
  • lstu as private
  • bozon as private,
  • jirafeau
  • lufi

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Jun 24, 2018

I would be interested in having the feedback from the apps team as they are the first concerned, therefor pinging them cc @frju365 @JimboJoe @Josue-T @maniackcrudelis (or at the one I know to be actives)

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Jun 25, 2018

I had implemented the lua pattern in gogs and I confirm that it' a pain to use it. So I think that it's a really good idea to use PCRE instead.

@Psycojoker

This comment has been minimized.

Copy link
Member

Psycojoker commented Jun 26, 2018

@maniackcrudelis said on the chat that this was a good idea too and that it has already been discussed in the past apparently.

I'm personally a bit weirded out by this dual match approach, I hope this won't create weird bugs :/ Maybe we should have something like explicitely specifying the regex type? Like starting the regex string with "pcre:" and "luare:" (default)?

Otherwise, LGTM.

@JimboJoe
Copy link
Member

JimboJoe left a comment

This is very LGTM as well! 👍

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Aug 15, 2018

Bumpitybump : what's the status of this ? Has it been tested ? Should it be merged ?

@zamentur

This comment has been minimized.

Copy link
Contributor Author

zamentur commented Sep 20, 2018

I suggest to put it in a testing release

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Sep 20, 2018

Add support of PCRE, and keep support of lua pattern during a transition period.

Would there be a "simple" way to detect if the user is using Lua regexed or PCRE regexes ? So far it essentially looks like this will support both - but if we really want to push the usage to PCRE, we shall warn packagers about Lua regexes being deprecated somehow ?

This doesn't prevent from merging this though (I would be happy to merge this right away and include this in 3.3 testing)

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Oct 27, 2018

Discussing this with ljf today : there are not so many apps using those regexes in fact (c.f. list in original post) so it's not so important to check. But ljf will investigate a bit more the question of identifying lua regexes

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Nov 27, 2018

But ljf will investigate a bit more the question of identifying lua regexes

Bump @zamentur ;)

(Otherwise we can also to just yolomerge this as it is, i dunno...)

@zamentur

This comment has been minimized.

Copy link
Contributor Author

zamentur commented Dec 13, 2018

Tested with the %.well%-known/acme%-challenge ssowat rules

@alexAubin alexAubin merged commit c272b4c into stretch-unstable Dec 13, 2018

@alexAubin alexAubin deleted the enh-pcre branch Dec 13, 2018

@alexAubin alexAubin added this to the 3.4.x milestone Dec 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment