Issue 4826294168584192: Issue 2249 - Support UDP sockets in multiplexer.fcgi (Closed)
|
Created:
March 31, 2015, 7:02 p.m. by mathias Modified:
April 1, 2015, 8:59 p.m. CC:
Fred Visibility:
Public. |
DescriptionSee https://issues.adblockplus.org/ticket/2249 for more information..
Patch Set 1 #
Total comments: 6
Patch Set 2 : Issue 2249 - Support UDP sockets in multiplexer.fcgi #Patch Set 3 : Issue 2249 - Support UDP sockets in multiplexer.fcgi #MessagesTotal messages: 10
LGTM
http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... File multiplexer.fcgi (right): http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... multiplexer.fcgi:24: bindAddress = os.environ.get('FCGI_BIND_ADDRESS', None) Nit: .get() defaults to None anyway. So there is no need to specify a custom default value here. http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... multiplexer.fcgi:26: match = re.match(r'^(.*?):(\d+)$', bindAddress) Didn't we agree to don't use re.match() anymore in new code due to it's counter intuitive behavior, but re.search() instead?
http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... File multiplexer.fcgi (right): http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... multiplexer.fcgi:26: match = re.match(r'^(.*?):(\d+)$', bindAddress) On 2015/04/01 07:43:27, Sebastian Noack wrote: > Didn't we agree to don't use re.match() anymore in new code due to it's counter > intuitive behavior, but re.search() instead? What's about this?
http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... File multiplexer.fcgi (right): http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... multiplexer.fcgi:24: bindAddress = os.environ.get('FCGI_BIND_ADDRESS', None) On 2015/04/01 07:43:27, Sebastian Noack wrote: > Nit: .get() defaults to None anyway. So there is no need to specify a custom > default value here. Done. http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... multiplexer.fcgi:26: match = re.match(r'^(.*?):(\d+)$', bindAddress) On 2015/04/01 07:43:27, Sebastian Noack wrote: > Didn't we agree to don't use re.match() anymore in new code due to it's counter > intuitive behavior, but re.search() instead? Frankly I have no idea what you mean by that. What behavior? When did that happen? Who is we?
http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... File multiplexer.fcgi (right): http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... multiplexer.fcgi:26: match = re.match(r'^(.*?):(\d+)$', bindAddress) On 2015/04/01 08:03:11, matze wrote: > On 2015/04/01 07:43:27, Sebastian Noack wrote: > > Didn't we agree to don't use re.match() anymore in new code due to it's > counter > > intuitive behavior, but re.search() instead? > > Frankly I have no idea what you mean by that. > What behavior? When did that happen? Who is we? I'm talking about this discussion: http://codereview.adblockplus.org/5809999318089728/patch/5718998062727168/568... re.match(...) is equivalent to re.search('^' + ...). And both Felix and Wladimir were irritated by that behavior, that it is implicitly matching start but not end of string. So we decided to don't use it anymore.
On 2015/04/01 08:11:27, Sebastian Noack wrote: > http://codereview.adblockplus.org/4826294168584192/diff/5629499534213120/mult... > multiplexer.fcgi:26: match = re.match(r'^(.*?):(\d+)$', bindAddress) > On 2015/04/01 08:03:11, matze wrote: > > On 2015/04/01 07:43:27, Sebastian Noack wrote: > > > Didn't we agree to don't use re.match() anymore in new code due to it's > > counter > > > intuitive behavior, but re.search() instead? > > > > Frankly I have no idea what you mean by that. > > What behavior? When did that happen? Who is we? > > I'm talking about this discussion: > http://codereview.adblockplus.org/5809999318089728/patch/5718998062727168/568... > > re.match(...) is equivalent to re.search('^' + ...). And both Felix and Wladimir > were irritated by that behavior, that it is implicitly matching start but not > end of string. So we decided to don't use it anymore. Thank you for the update. In theory there is no change in behavior when using re.search() here, because of the explicit ^ and the absence multi-line considerations. However, "always use re.search()" is a pretty good practice.
See https://issues.adblockplus.org/ticket/2249 for more information..
LGTM 
|
