Add pattern matches to values for Mustermann::Concat. Fixes #1332 #1333

dometto · 2017-08-15T11:57:35Z

This resolves #1332. Two questions:

should I add a regression spec for this?
is it possible that matches should also be added to values? for classes other than Mustermann::Regular and Mustermann:Concat?

namusyaka · 2017-08-19T14:46:18Z

The :captures parameter is added for use in regexp route.
Also, the mustermann concat feature has been using by other routes like sinatra-namespace, so this patch makes params have the :captures params in unexpected routes.

dometto · 2017-08-20T10:08:38Z

The :captures parameter is added for use in regexp route.

Yes, but the problem seems to be exactly that :captures should also be added, in some cases at least, to concat routes (when the Mustermann::Concat consists in part of a regexp).

namusyaka · 2017-08-20T11:20:04Z

@dometto Could you add a test?

zzak · 2017-10-29T05:18:56Z

Needs a test

dometto · 2018-01-14T18:30:42Z

Sorry for the slow response. I've added a test (which indeed fails when the fix is absent).

namusyaka · 2018-01-15T10:48:07Z

Thanks for adding the test. I've just confirmed that.

However, I can't aggressively agree with this change.
Because the :captures parameter is required only if Mustermann::Concat consists of multiple patterns containing a Mustermann::Regular pattern.
In other words, Mustermann::Concat doesn't require captures except that case.

For example, one Mustermann::Concat may consist of Mustermann::Sinatra and Mustermann::SimpleMatch. In this case, obviously, the :captures parameter will be an empty array.

Although it seems that this change doesn't largely destroy backward compatibility (the :captures key is originally prohibited), this may confuse application developers in some cases.

namusyaka · 2018-01-15T10:49:57Z

Additional notes:
I have grasped the problem you indicated.
We need to work on this in some way and I think that the patch you sent is also one of the solutions.
Thanks!

dometto · 2018-01-15T11:19:51Z

Is there already an easy way to test if a Mustermann::Concat contains a regexp, e.g. .include?? In that case we can just make the condition more specific.

namusyaka · 2018-01-15T11:40:19Z

Nope, that's not the proper way.
However, you can determine by using Mustermann::Composite#patterns

concat = Mustermann.new("/prefix", type: :identity) + Mustermann.new('/foo')
p concat.patterns.any?{|pattern| pattern.is_a?(Mustermann::Regular) }
concat = Mustermann.new("/prefix", type: :regexp) + Mustermann.new('/foo')
p concat.patterns.any?{|pattern| pattern.is_a?(Mustermann::Regular) }

dometto · 2018-01-15T13:19:22Z

Fair enough. 👍 I'll put together a fix based on that!

dometto · 2018-01-15T13:58:34Z

New implementation. The check in base.rb is now more complicated, but I think it performs well and is precise enough. If you have any suggestions on how to shorten it or make it more readable, let me know.

namusyaka

Looks good with a few nits.
Thanks.

namusyaka · 2018-01-15T14:03:43Z

lib/sinatra/base.rb

@@ -1024,7 +1024,9 @@ def process_route(pattern, conditions, block = nil, values = [])
      params.delete("ignore") # TODO: better params handling, maybe turn it into "smart" object or detect changes
      original, @params = @params, @params.merge(params) if params.any?

-      if pattern.is_a?(Mustermann::Regular) || pattern.is_a?(Mustermann::Concat)
+      has_regexp = pattern.is_a?(Mustermann::Regular) ? true : 
+        (pattern.respond_to?(:patterns) && pattern.patterns.any? {|subpattern| subpattern.is_a?(Mustermann::Regular)} )


prefer AND operator over conditional operator in this case.

regexp_exists = pattern.is_a?(Mustermann::Regular) || (pattern.respond_to?(:patterns) && pattern.patterns.any? {|subpattern| subpattern.is_a?(Mustermann::Regular)} )

namusyaka · 2018-01-15T14:07:07Z

test/routing_test.rb

@@ -630,17 +630,26 @@ class RoutingTest < Minitest::Test
  end

  it 'makes regular expression captures available in params[:captures] for concatenated routes' do
-    concatenated = Mustermann.new('/prefix') + Mustermann.new("/fo(.*)/ba(.*)", :type => :regexp)
+    with_regexp = Mustermann.new('/prefix') + Mustermann.new("/fo(.*)/ba(.*)", :type => :regexp)
+    without_regexp = Mustermann.new('/prefix') + Mustermann.new('/baz')


Maybe this is not Mustermann::Concat, but pure Mustermann::Sinatra.
You should use multiple mustermann types, it will be something like:

without_regexp = Mustermann.new('/prefix', type: :identity) + Mustermann.new('/baz')

namusyaka · 2018-01-15T14:07:42Z

test/routing_test.rb

@@ -630,17 +630,26 @@ class RoutingTest < Minitest::Test
  end

  it 'makes regular expression captures available in params[:captures] for concatenated routes' do
-    concatenated = Mustermann.new('/prefix') + Mustermann.new("/fo(.*)/ba(.*)", :type => :regexp)
+    with_regexp = Mustermann.new('/prefix') + Mustermann.new("/fo(.*)/ba(.*)", :type => :regexp)


Please use keyword argument instead of hash-rocket.

dometto · 2018-01-15T14:50:21Z

👍

namusyaka · 2018-01-15T14:51:47Z

Perfect! Please wait for cutting next release!

zzak · 2018-01-16T02:08:22Z

Could you squash your commits please? 🙏

…ntains regexps.

dometto · 2018-01-16T12:06:08Z

All ready!

dometto · 2018-01-31T18:02:51Z

Any idea when this will be merged/released?

namusyaka · 2018-02-06T15:26:46Z

Sorry for the late reply. Now I don't see any reason why we cannot merge this.

Thank you for your contribution!

zzak added the help wanted label Oct 29, 2017

namusyaka reviewed Jan 15, 2018

View reviewed changes

namusyaka mentioned this pull request Jan 15, 2018

v2.0.1 #1339

Closed

Add caputured patterns to params for Mustermann::Concat in case it co…

4c89cba

…ntains regexps.

dometto force-pushed the fix1332 branch from 1121a87 to 4c89cba Compare January 16, 2018 12:05

namusyaka added this to the v2.0.1 milestone Feb 6, 2018

namusyaka merged commit a96f144 into sinatra:master Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pattern matches to values for Mustermann::Concat. Fixes #1332 #1333

Add pattern matches to values for Mustermann::Concat. Fixes #1332 #1333

dometto commented Aug 15, 2017

namusyaka commented Aug 19, 2017

dometto commented Aug 20, 2017

namusyaka commented Aug 20, 2017

zzak commented Oct 29, 2017

dometto commented Jan 14, 2018

namusyaka commented Jan 15, 2018

namusyaka commented Jan 15, 2018

dometto commented Jan 15, 2018

namusyaka commented Jan 15, 2018

dometto commented Jan 15, 2018

dometto commented Jan 15, 2018

namusyaka left a comment

namusyaka Jan 15, 2018

namusyaka Jan 15, 2018 •

edited

Loading

namusyaka Jan 15, 2018

dometto commented Jan 15, 2018

namusyaka commented Jan 15, 2018

zzak commented Jan 16, 2018

dometto commented Jan 16, 2018

dometto commented Jan 31, 2018

namusyaka commented Feb 6, 2018

Add pattern matches to values for Mustermann::Concat. Fixes #1332 #1333

Add pattern matches to values for Mustermann::Concat. Fixes #1332 #1333

Conversation

dometto commented Aug 15, 2017

namusyaka commented Aug 19, 2017

dometto commented Aug 20, 2017

namusyaka commented Aug 20, 2017

zzak commented Oct 29, 2017

dometto commented Jan 14, 2018

namusyaka commented Jan 15, 2018

namusyaka commented Jan 15, 2018

dometto commented Jan 15, 2018

namusyaka commented Jan 15, 2018

dometto commented Jan 15, 2018

dometto commented Jan 15, 2018

namusyaka left a comment

Choose a reason for hiding this comment

namusyaka Jan 15, 2018

Choose a reason for hiding this comment

namusyaka Jan 15, 2018 • edited Loading

Choose a reason for hiding this comment

namusyaka Jan 15, 2018

Choose a reason for hiding this comment

dometto commented Jan 15, 2018

namusyaka commented Jan 15, 2018

zzak commented Jan 16, 2018

dometto commented Jan 16, 2018

dometto commented Jan 31, 2018

namusyaka commented Feb 6, 2018

namusyaka Jan 15, 2018 •

edited

Loading