Translate /i regexes to RE2 regex syntax #120

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@mbrock
Contributor

mbrock commented Jan 14, 2015

Fixes #119.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

/m ?

Owner

nviennot commented Jan 14, 2015

/m ?

lib/no_brainer/criteria/where.rb
else instantiate_binary_op(key, :eq, value)
end
end
+ def translate_regexp_to_re2_syntax(value)
+ options = ""
+ options << "i" if 0 != value.options & Regexp::IGNORECASE

This comment has been minimized.

@nviennot

nviennot Jan 14, 2015

Owner

0 != x -> x != 0 (yoda comparison otherwise)

@nviennot

nviennot Jan 14, 2015

Owner

0 != x -> x != 0 (yoda comparison otherwise)

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 14, 2015

Contributor

Excellent question. I couldn't actually figure out what's going on with that flag.

From the RE2 syntax reference:

m   multi-line mode: ^ and $ match begin/end line in addition to begin/end text (default false)
s   let . match \n (default false)

But in my testing, it seemed as if these options were defaulting to true, though I couldn't see anything in the RethinkDB source code to indicate this... I got confused. Let me play around a bit more.

BTW, note that currently, the regex foo/i compiles into the string "foo/", which is broken.

Contributor

mbrock commented Jan 14, 2015

Excellent question. I couldn't actually figure out what's going on with that flag.

From the RE2 syntax reference:

m   multi-line mode: ^ and $ match begin/end line in addition to begin/end text (default false)
s   let . match \n (default false)

But in my testing, it seemed as if these options were defaulting to true, though I couldn't see anything in the RethinkDB source code to indicate this... I got confused. Let me play around a bit more.

BTW, note that currently, the regex foo/i compiles into the string "foo/", which is broken.

lib/no_brainer/criteria/where.rb
+ if options.empty?
+ value.source
+ else
+ "(?#{options}:#{value.source})"

This comment has been minimized.

@nviennot

nviennot Jan 14, 2015

Owner

From https://code.google.com/p/re2/wiki/Syntax it seems that we should rename options to `flags.

From the example on http://www.rethinkdb.com/api/ruby/match/ it seems that the parenthesis go around the flags, not the whole thing.

From the re2 docs:

(?flags) set flags within current group; non-capturing
(?flags:re) set flags during re; non-capturing

I don't understand the difference. Do you?

@nviennot

nviennot Jan 14, 2015

Owner

From https://code.google.com/p/re2/wiki/Syntax it seems that we should rename options to `flags.

From the example on http://www.rethinkdb.com/api/ruby/match/ it seems that the parenthesis go around the flags, not the whole thing.

From the re2 docs:

(?flags) set flags within current group; non-capturing
(?flags:re) set flags during re; non-capturing

I don't understand the difference. Do you?

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 14, 2015

Contributor

Good points. Regarding the flags syntax, I don't think there is a difference in this case, but I could imagine that using the former might possibly give better error messages in some cases, or something.

Contributor

mbrock commented Jan 14, 2015

Good points. Regarding the flags syntax, I don't think there is a difference in this case, but I could imagine that using the former might possibly give better error messages in some cases, or something.

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 14, 2015

Contributor

I've assumed that we want to ensure that Regexp#match matches the behavior of the RethinkDB query. This means always adding the m flag.

Contributor

mbrock commented Jan 14, 2015

I've assumed that we want to ensure that Regexp#match matches the behavior of the RethinkDB query. This means always adding the m flag.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

your PR looks good to me, and I confirm your findings.

also, I wanted to add a "match" keyword to be explicit so that we can pass a raw RE2 string, but I can't define a match() method on Symbol it seems :(

Owner

nviennot commented Jan 14, 2015

your PR looks good to me, and I confirm your findings.

also, I wanted to add a "match" keyword to be explicit so that we can pass a raw RE2 string, but I can't define a match() method on Symbol it seems :(

@nviennot nviennot closed this in 2cf8e75 Jan 14, 2015

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

Thank you :) :) :)

Owner

nviennot commented Jan 14, 2015

Thank you :) :) :)

@mbrock

This comment has been minimized.

Show comment
Hide comment
@mbrock

mbrock Jan 14, 2015

Contributor

Cool! By the way, I discovered this because rails_admin generates case-insensitive regexes for searching. It's quite fun to see rails_admin working with nobrainer. I'm finishing up the adapter for searching and filtering now. 😄

Contributor

mbrock commented Jan 14, 2015

Cool! By the way, I discovered this because rails_admin generates case-insensitive regexes for searching. It's quite fun to see rails_admin working with nobrainer. I'm finishing up the adapter for searching and filtering now. 😄

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jan 14, 2015

Owner

Well done!! I don't see your rails_admin fork though, where is it?

BTW, The RethinkDB guys will probably be happy to showcase it

Owner

nviennot commented Jan 14, 2015

Well done!! I don't see your rails_admin fork though, where is it?

BTW, The RethinkDB guys will probably be happy to showcase it

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