Mailing-List: contact deltacloud-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: deltacloud-dev@incubator.apache.org
Received-SPF: pass (nike.apache.org: domain of clalance@redhat.com designates
 209.132.183.28 as permitted sender)
Date: Mon, 6 Jun 2011 12:15:55 -0400
From: Chris Lalancette <clalance@redhat.com>
To: deltacloud-dev@incubator.apache.org
Subject: Re: [PATCH core 1/3] Added 'api_url_for' helper to deal with prefix
 and matrix parameters as 'driver' and 'provider'. Every link should now
 include those parameters when they are present.
Message-ID: <20110606161554.GC12512@localhost.localdomain>
Reply-To: Chris Lalancette <clalance@redhat.com>
References: <1307374515-40207-1-git-send-email-mfojtik@redhat.com>
 <1307374515-40207-2-git-send-email-mfojtik@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1307374515-40207-2-git-send-email-mfojtik@redhat.com>
User-Agent: Mutt/1.5.21 (2010-09-15)

On 06/06/11 - 05:35:13PM, mfojtik@redhat.com wrote:
> From: Chris Lalancette <clalance@redhat.com>
> 
> This patch also introduces DEFAULT_URI_PREFIX constant to be use
> as a default for all routes instead of '/api
> 
> Signed-off-by: Michal fojtik <mfojtik@redhat.com>
> ---
>  server/lib/sinatra/rack_matrix_params.rb |    2 +-
>  server/lib/sinatra/url_for.rb            |   21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 2 deletions(-)

Sweet, this seems to work for me, and is a better idea overall.  Two nits...

> 
> diff --git a/server/lib/sinatra/rack_matrix_params.rb b/server/lib/sinatra/rack_matrix_params.rb
> index 6362c7f..da1889e 100644
> --- a/server/lib/sinatra/rack_matrix_params.rb
> +++ b/server/lib/sinatra/rack_matrix_params.rb
> @@ -57,7 +57,7 @@ module Rack
>  	    value=nil
>  	    next
>  	  else
> -	    value = param
> +	    value = param.gsub(/\?.*$/, '')
>  	  end
>  	end
>        end
> diff --git a/server/lib/sinatra/url_for.rb b/server/lib/sinatra/url_for.rb
> index 60da0ef..4bd24ae 100644
> --- a/server/lib/sinatra/url_for.rb
> +++ b/server/lib/sinatra/url_for.rb
> @@ -28,6 +28,19 @@ require 'uri'
>  
>  module Sinatra
>    module UrlForHelper
> +
> +    DEFAULT_URI_PREFIX = "/api"
> +
> +    def api_url_for(url_fragment, mode=:path_only)
> +      matrix_params = ''
> +      if request.params['api']
> +        matrix_params += ";provider=%s" % request.params['api']['provider'] if request.params['api']['provider']
> +        matrix_params += ";driver=%s" % request.params['api']['driver'] if request.params['api']['driver']
> +      end
> +      url_fragment = "/#{url_fragment}" unless url_fragment =~ /^\// # There is no need to prefix URI with '/'
> +      url_for "#{DEFAULT_URI_PREFIX}#{matrix_params}#{url_fragment}", mode
> +    end
> +
>      # Construct a link to +url_fragment+, which should be given relative to
>      # the base of this Sinatra app.  The mode should be either
>      # <code>:path_only</code>, which will generate an absolute path within
> @@ -63,14 +76,20 @@ module Sinatra
>        # Don't add the base fragment if url_for gets called more than once
>        # per url or the url_fragment passed in is an absolute url
>        if url_escape.match(/^#{base}/) or url_escape.match(/^http/)
> +        # here, we assume that if any driver additions are needed, they have
> +        # been done prior to entering this method.  Hence we just return the
> +        # URL as-is
>          url_escape

This comment no longer makes sense.

>        else
> +        # one last thing we need to do here is to make sure we properly add
> +        # the driver type to the URL (if necessary)
> +        # puts request.env['REQUEST_URI'].inspect

Nor does this.  I would suggest just dropping the comments completely.

-- 
Chris Lalancette