-
Notifications
You must be signed in to change notification settings - Fork 36
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
Added support for reverse variable matching #54
Added support for reverse variable matching #54
Conversation
bcf9ad3
to
05b8e06
Compare
Bumped Java version to 8
05b8e06
to
caef7c7
Compare
Thanks! |
.append('?'); | ||
regex.deleteCharAt(regex.length() -1); | ||
|
||
Pattern pattern = Pattern.compile(regex.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to get reasonable performance, this pattern will need to be precompiled so it can be used over and over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quite right, this and the pattern in matchParameters
should be compiled late and once.
.append('>') | ||
.append('\\') | ||
.append(prefix) | ||
.append(".{1,9001}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for limiting this to 9001 characters? You can use .+
if you want to match at least one character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there was an issue with unbounded match. It would cause a stack overflow IIRC. 9001 is sufficiently large for a single sane expression, and investigating and documenting the phenomenon more is intended to fall under the todo
.
@@ -82,6 +85,14 @@ | |||
*/ | |||
private Pattern matchPattern; | |||
|
|||
private String groupName = uid(); | |||
|
|||
private static Random RANDOM = new SecureRandom(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SecureRandom
is a very expensive way to generate random names, in particular it's blocking, only one thread can generate a name at a time, and depending on your JVM configuration, it can block indefinitely if your system has no entropy. If the aim is to ensure that each name this class generates is unique within the scope of this class, then I'd just use an AtomicLong
, and generate names like var1
, var2
, var3
using incrementAndGet
. This is fast, non blocking, and guarantees uniqueness up to Long.MAX_VALUE
names (technically there's no such guarantee with SecureRandom
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good point. My original thought was that the collision chance of 12 bytes was practically none, and a counter could max out on a long running server.
Now that you bring it up however, I think the performance hit of random is greater than the practical likelihood of using over 8 bytes of expressions (and if it becomes an issue, later code could handle it by resetting the counter or creating new counters and appending them together as strings).
Addresses #2
Bumped Java version to 8
Added
Either
monad class to deal with repeated variables.Currently this should support the majority of use cases, however more work can be done to better support level 4 templates.
Future work:
matchParameters
andmatchSegments
inspect eachVarSpec
for allowed charactersmatchParameters
andmatchSegments
inspect eachVarSpec
for explode modifierbuildMatchingPattern
match more strictly, seems to work OK now but could result in issue reports later