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

Fix namespace extension to support latest Sinatra version #207

Merged
merged 2 commits into from Jul 16, 2016

Conversation

Projects
None yet
3 participants
@304
Contributor

304 commented Jul 15, 2016

This fix should resolve some of the issues with namespace extension.

I haven't worked with this extension before, so I was completely relying on tests during my investigation. Please feel free to comment.

Issue: #200

@stjhimy

This comment has been minimized.

Show comment
Hide comment
@stjhimy

stjhimy Jul 15, 2016

Contributor

Amazing!
You must require mustermann somewhere in order to pass all the checks, that's because it is required only on sinatra/master.

See the errors on travis:
https://travis-ci.org/sinatra/sinatra-contrib/builds/145003731

Contributor

stjhimy commented Jul 15, 2016

Amazing!
You must require mustermann somewhere in order to pass all the checks, that's because it is required only on sinatra/master.

See the errors on travis:
https://travis-ci.org/sinatra/sinatra-contrib/builds/145003731

@304

This comment has been minimized.

Show comment
Hide comment
@304

304 Jul 15, 2016

Contributor

Thanks @stjhimy! The mustermann error is gone.
However, there is still an issue with splat. And looks like it is caused by sinatra.

sinatra=stable bundle exec rake

> 845 examples, 7 failures, 14 pending

But the latest version of sinatra contains a fix for it.

sinatra=master bundle exec rake

> 845 examples, 0 failures, 14 pending

What do you think is the best way to resolve it?

Contributor

304 commented Jul 15, 2016

Thanks @stjhimy! The mustermann error is gone.
However, there is still an issue with splat. And looks like it is caused by sinatra.

sinatra=stable bundle exec rake

> 845 examples, 7 failures, 14 pending

But the latest version of sinatra contains a fix for it.

sinatra=master bundle exec rake

> 845 examples, 0 failures, 14 pending

What do you think is the best way to resolve it?

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Jul 15, 2016

Member

zdfnaksdjfasjdf;laskdjfpoiasdjfasdf

THANK YOU!!!

I can't believe the fix was this easy, and have been pining over this for literally weeks.

Member

zzak commented Jul 15, 2016

zdfnaksdjfasjdf;laskdjfpoiasdjfasdf

THANK YOU!!!

I can't believe the fix was this easy, and have been pining over this for literally weeks.

@zzak zzak merged commit dd3e6c3 into sinatra:master Jul 16, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Jul 16, 2016

Member

@304 @stjhimy Ahh, yeah there is an issue when using regular expression in the path.

If we don't skip spec/namespace_spec.rb:82 and :101, I get the following failures still:

Failures:

  1) Sinatra::Namespace HTTP GET when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  2) Sinatra::Namespace HTTP GET when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  3) Sinatra::Namespace HTTP HEAD when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  4) Sinatra::Namespace HTTP HEAD when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  5) Sinatra::Namespace HTTP POST when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  6) Sinatra::Namespace HTTP POST when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  7) Sinatra::Namespace HTTP PUT when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  8) Sinatra::Namespace HTTP PUT when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  9) Sinatra::Namespace HTTP DELETE when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  10) Sinatra::Namespace HTTP DELETE when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  11) Sinatra::Namespace HTTP OPTIONS when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  12) Sinatra::Namespace HTTP OPTIONS when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  13) Sinatra::Namespace HTTP PATCH when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  14) Sinatra::Namespace HTTP PATCH when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

Finished in 2.6 seconds   
845 examples, 14 failures

Failed examples:

rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP GET when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP GET when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP HEAD when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP HEAD when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP POST when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP POST when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP PUT when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP PUT when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP DELETE when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP DELETE when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP OPTIONS when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP OPTIONS when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP PATCH when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP PATCH when namespace is a regular expression accepts the path as a named parameter
Member

zzak commented Jul 16, 2016

@304 @stjhimy Ahh, yeah there is an issue when using regular expression in the path.

If we don't skip spec/namespace_spec.rb:82 and :101, I get the following failures still:

Failures:

  1) Sinatra::Namespace HTTP GET when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  2) Sinatra::Namespace HTTP GET when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  3) Sinatra::Namespace HTTP HEAD when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  4) Sinatra::Namespace HTTP HEAD when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  5) Sinatra::Namespace HTTP POST when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  6) Sinatra::Namespace HTTP POST when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  7) Sinatra::Namespace HTTP PUT when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  8) Sinatra::Namespace HTTP PUT when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  9) Sinatra::Namespace HTTP DELETE when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  10) Sinatra::Namespace HTTP DELETE when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  11) Sinatra::Namespace HTTP OPTIONS when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  12) Sinatra::Namespace HTTP OPTIONS when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

  13) Sinatra::Namespace HTTP PATCH when namespace is a named parameter accepts the path as regular expression
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:86:in `block (5 levels) in <top (required)>'

  14) Sinatra::Namespace HTTP PATCH when namespace is a regular expression accepts the path as a named parameter
     Failure/Error: send(verb, '/foo/bar').should be_ok
       expected ok? to return true, got false
     # ./spec/namespace_spec.rb:105:in `block (5 levels) in <top (required)>'

Finished in 2.6 seconds   
845 examples, 14 failures

Failed examples:

rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP GET when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP GET when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP HEAD when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP HEAD when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP POST when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP POST when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP PUT when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP PUT when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP DELETE when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP DELETE when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP OPTIONS when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP OPTIONS when namespace is a regular expression accepts the path as a named parameter
rspec ./spec/namespace_spec.rb:82 # Sinatra::Namespace HTTP PATCH when namespace is a named parameter accepts the path as regular expression
rspec ./spec/namespace_spec.rb:101 # Sinatra::Namespace HTTP PATCH when namespace is a regular expression accepts the path as a named parameter
@stjhimy

This comment has been minimized.

Show comment
Hide comment
@stjhimy

stjhimy Jul 18, 2016

Contributor

@zzak yes, this is the one we must patch mustermann and add support for /:foo/ + %r{/bar} in order to work.

Contributor

stjhimy commented Jul 18, 2016

@zzak yes, this is the one we must patch mustermann and add support for /:foo/ + %r{/bar} in order to work.

zzak added a commit that referenced this pull request Jul 19, 2016

Use Mustermann from master until a release is made
Since master has a patch which fixes the missing `:+` implementation for
Regular and Sinatra-based patterns.

We can also re-enable the namespace specs for accepting regular expressions for
path concatination.

/cc #200 and #207
@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Jul 19, 2016

Member

@stjhimy That is no longer an issue, when we use the master branch on mustermann (until the patch is released).

The remaining blockers are described in 2d2a445

Member

zzak commented Jul 19, 2016

@stjhimy That is no longer an issue, when we use the master branch on mustermann (until the patch is released).

The remaining blockers are described in 2d2a445

zzak added a commit to zzak/sinatra-contrib that referenced this pull request Jul 22, 2016

Merge pull request #207 from 304/fix_namespace_extension
Fix namespace extension to support latest Sinatra version

zzak added a commit to zzak/sinatra-contrib that referenced this pull request Jul 22, 2016

Use Mustermann from master until a release is made
Since master has a patch which fixes the missing `:+` implementation for
Regular and Sinatra-based patterns.

We can also re-enable the namespace specs for accepting regular expressions for
path concatination.

/cc #200 and #207

zzak added a commit to zzak/sinatra-contrib that referenced this pull request Jul 22, 2016

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