rack-devel archive mirror (unofficial) https://groups.google.com/group/rack-devel
 help / color / mirror / code / Atom feed
* Potential Rack-Contrib contributions
@ 2010-01-15 16:23 Geoff Buesing
  2010-01-16 12:29 ` Ryan Tomayko
  2010-01-16 13:22 ` Iñaki Baz Castillo
  0 siblings, 2 replies; 9+ messages in thread
From: Geoff Buesing @ 2010-01-15 16:23 UTC (permalink / raw)
  To: Rack Development

I've pulled together some useful generic middleware into
http://github.com/gbuesing/rackables:

* Rackables::Branch - Conditionally re-routes the Rack stack at
runtime to an alternate endpoint
* Rackables::CacheControl - Sets response Cache-Control header
* Rackables::DefaultCharset - Sets charset directive in Content-Type
header
* Rackables::Get - Allows creation of simple endpoints and path
routing with a syntax similar to Sinatra's get method
* Rackables::HideExceptions - Rescues exceptions with a static
exception page
* Rackables::TrailingSlashRedirect - 301 Redirects requests paths with
a trailing slash

If any of these would be a good fit for Rack-Contrib, let me know, I'd
be happy to pull together patches.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-15 16:23 Potential Rack-Contrib contributions Geoff Buesing
@ 2010-01-16 12:29 ` Ryan Tomayko
  2010-01-16 13:22 ` Iñaki Baz Castillo
  1 sibling, 0 replies; 9+ messages in thread
From: Ryan Tomayko @ 2010-01-16 12:29 UTC (permalink / raw)
  To: rack-devel

On Fri, Jan 15, 2010 at 8:23 AM, Geoff Buesing <gbuesing@gmail.com> wrote:
> I've pulled together some useful generic middleware into
> http://github.com/gbuesing/rackables:
>
> * Rackables::Branch - Conditionally re-routes the Rack stack at
> runtime to an alternate endpoint
> * Rackables::CacheControl - Sets response Cache-Control header
> * Rackables::DefaultCharset - Sets charset directive in Content-Type
> header
> * Rackables::Get - Allows creation of simple endpoints and path
> routing with a syntax similar to Sinatra's get method
> * Rackables::HideExceptions - Rescues exceptions with a static
> exception page
> * Rackables::TrailingSlashRedirect - 301 Redirects requests paths with
> a trailing slash
>
> If any of these would be a good fit for Rack-Contrib, let me know, I'd
> be happy to pull together patches.

You have some great stuff in there.

So far, the rack-contrib policy has been to accept anything that's
generally useful, has tests, and is more or less documented. All of
the above appear to pass those tests.

The only thing I would have a problem with is Rackables::Get -- the
name is just too general. How about SimpleGetEndpoint or something
similarly verbose. Classname brevity isn't a feature when it comes to
rack-contrib.

If you want to roll these into one rack-contrib patch per middleware,
I'd be very happy to review and merge them.

Thanks,
Ryan

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-15 16:23 Potential Rack-Contrib contributions Geoff Buesing
  2010-01-16 12:29 ` Ryan Tomayko
@ 2010-01-16 13:22 ` Iñaki Baz Castillo
  2010-01-16 13:38   ` Iñaki Baz Castillo
  1 sibling, 1 reply; 9+ messages in thread
From: Iñaki Baz Castillo @ 2010-01-16 13:22 UTC (permalink / raw)
  To: rack-devel

El Viernes, 15 de Enero de 2010, Geoff Buesing escribió:
> * Rackables::DefaultCharset - Sets charset directive in Content-Type
> header

Hi, I'm inspecting DefaultCharset middleware:


  class DefaultCharset
    HAS_CHARSET = /charset=/
 
    def initialize(app, value = 'utf-8')
      @app = app
      @value = value
    end
 
    def call(env)
      response = @app.call(env)
      headers = response[1]
      content_type = headers['Content-Type']
      if content_type && content_type !~ HAS_CHARSET
        headers['Content-Type'] = "#{content_type}; charset=#{@value}"
      end
      response
    end
  end


I see two issues here:

1) HAS_CHARSET = /charset=/
Note that HTTP grammar is case insensitive (in most of the cases) so IMGO it 
shoud be:

  HAS_CHARSET = /charset=/i

2) content_type = headers['Content-Type']
Perhaps I'm wrong, but what about if the response headers created by other 
middleware or Rack app uses "content-type" or "CONTENT-type"? again RFC 2616 
BNF grammar is case insensitive by default.

However in case 2 perhaps I'm wrong as I don't know if Rack require some exact 
format for headers name into generated responses.


Regards.



-- 
Iñaki Baz Castillo <ibc@aliax.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-16 13:22 ` Iñaki Baz Castillo
@ 2010-01-16 13:38   ` Iñaki Baz Castillo
  2010-01-16 14:01     ` Michael Fellinger
  0 siblings, 1 reply; 9+ messages in thread
From: Iñaki Baz Castillo @ 2010-01-16 13:38 UTC (permalink / raw)
  To: rack-devel

El Sábado, 16 de Enero de 2010, Iñaki Baz Castillo escribió:

> 1) HAS_CHARSET = /charset=/
> Note that HTTP grammar is case insensitive (in most of the cases) so IMGO
>  it shoud be:
> 
>   HAS_CHARSET = /charset=/i


Humm, definitively it's not enough. HTTP BNF grammar is really more complex 
that the above hyper simplified assumption.

The folllowing headers are all valid "Content-Type" headers, and all of them 
contain a valid "charset" param (replace <LF> with a real \r symbol and <TAB> 
with \t):


Content-Type: text/plain ; CharSet<LF>
  = utf-8


content-TYPE:text/plain ; CharSet<LF>
  = utf-8


Content-Type: text/plain <LF>
 ; CharSet <LF>
  = utf-8


Content-Type:<TAB><TAB>text/plain<TAB>; <LF>
 charset <LF>
<TAB><TAB>  <TAB> = <TAB><LF>
  utf-8


Well for sure we can assume that other Rack middlewwares and also our Rack app 
would generate a simple inline header value. Then I would rewrite the regex to 
this one (the ";" is important):

  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i


Regards.


-- 
Iñaki Baz Castillo <ibc@aliax.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-16 13:38   ` Iñaki Baz Castillo
@ 2010-01-16 14:01     ` Michael Fellinger
  2010-01-16 14:47       ` Iñaki Baz Castillo
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Fellinger @ 2010-01-16 14:01 UTC (permalink / raw)
  To: rack-devel

> Well for sure we can assume that other Rack middlewwares and also our Rack app
> would generate a simple inline header value. Then I would rewrite the regex to
> this one (the ";" is important):
>
>  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i

"\t" =~ /\s/
# 0
means
/;\s*charset\s*=\s*/i
will be enough.

Regarding the case of Content-Type in the header Hash, as long as it's
an instance of HeaderHash, the case will not matter, we normalize it
behind the scene.

-- 
Michael Fellinger
CTO, The Rubyists, LLC

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-16 14:01     ` Michael Fellinger
@ 2010-01-16 14:47       ` Iñaki Baz Castillo
  2010-01-16 17:47         ` Geoff Buesing
  0 siblings, 1 reply; 9+ messages in thread
From: Iñaki Baz Castillo @ 2010-01-16 14:47 UTC (permalink / raw)
  To: rack-devel

El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
> > Well for sure we can assume that other Rack middlewwares and also our
> > Rack app would generate a simple inline header value. Then I would
> > rewrite the regex to this one (the ";" is important):
> >
> >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
> 
> "\t" =~ /\s/
> # 0
> means
> /;\s*charset\s*=\s*/i
> will be enough.

Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense 
as \s includes space and tabulator.


> Regarding the case of Content-Type in the header Hash, as long as it's
> an instance of HeaderHash, the case will not matter, we normalize it
> behind the scene.

ok, that's why I said "in case 2 perhaps I'm wrong" :)

Thanks a lot.


-- 
Iñaki Baz Castillo <ibc@aliax.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-16 14:47       ` Iñaki Baz Castillo
@ 2010-01-16 17:47         ` Geoff Buesing
  2010-01-17 18:32           ` Geoff Buesing
  0 siblings, 1 reply; 9+ messages in thread
From: Geoff Buesing @ 2010-01-16 17:47 UTC (permalink / raw)
  To: Rack Development


@Ryan - thanks, I'll pull together patches. And I agree that
Rackables::Get is to generic of a name; SimpleGetEndpoint is much
better. Running with that idea, I'm thinking of going a bit more
generic: Rack::SimpleEndpoint, with a verb requirement as an optional
argument.

@Iñaki and @Michael - thanks for the case-sensitivity heads up, and
the regex. I'll make sure to put in tests for these cases.

Geoff

On Jan 16, 8:47 am, Iñaki Baz Castillo <i...@aliax.net> wrote:
> El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
>
> > > Well for sure we can assume that other Rack middlewwares and also our
> > > Rack app would generate a simple inline header value. Then I would
> > > rewrite the regex to this one (the ";" is important):
>
> > >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
>
> > "\t" =~ /\s/
> > # 0
> > means
> > /;\s*charset\s*=\s*/i
> > will be enough.
>
> Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense
> as \s includes space and tabulator.
>
> > Regarding the case of Content-Type in the header Hash, as long as it's
> > an instance of HeaderHash, the case will not matter, we normalize it
> > behind the scene.
>
> ok, that's why I said "in case 2 perhaps I'm wrong" :)
>
> Thanks a lot.
>
> --
> Iñaki Baz Castillo <i...@aliax.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-16 17:47         ` Geoff Buesing
@ 2010-01-17 18:32           ` Geoff Buesing
  2010-01-17 19:15             ` Geoff Buesing
  0 siblings, 1 reply; 9+ messages in thread
From: Geoff Buesing @ 2010-01-17 18:32 UTC (permalink / raw)
  To: Rack Development

@Ryan - patches added as Lighthouse tickets:

http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/3-new-middleware-rackbranch
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/4-new-middleware-rackcachecontrol
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/5-new-middleware-rackdefaultcharset
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/6-new-middleware-rackhideexceptions
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/7-new-middleware-racksimpleendpoint
http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/8-rackenv-reader-and-rackenvdevelopment-etc-inquiry-methods

Thanks!

Geoff

On Jan 16, 11:47 am, Geoff Buesing <gbues...@gmail.com> wrote:
> @Ryan - thanks, I'll pull together patches. And I agree that
> Rackables::Get is to generic of a name; SimpleGetEndpoint is much
> better. Running with that idea, I'm thinking of going a bit more
> generic: Rack::SimpleEndpoint, with a verb requirement as an optional
> argument.
>
> @Iñaki and @Michael - thanks for the case-sensitivity heads up, and
> the regex. I'll make sure to put in tests for these cases.
>
> Geoff
>
> On Jan 16, 8:47 am, Iñaki Baz Castillo <i...@aliax.net> wrote:
>
>
>
> > El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
>
> > > > Well for sure we can assume that other Rack middlewwares and also our
> > > > Rack app would generate a simple inline header value. Then I would
> > > > rewrite the regex to this one (the ";" is important):
>
> > > >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
>
> > > "\t" =~ /\s/
> > > # 0
> > > means
> > > /;\s*charset\s*=\s*/i
> > > will be enough.
>
> > Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense
> > as \s includes space and tabulator.
>
> > > Regarding the case of Content-Type in the header Hash, as long as it's
> > > an instance of HeaderHash, the case will not matter, we normalize it
> > > behind the scene.
>
> > ok, that's why I said "in case 2 perhaps I'm wrong" :)
>
> > Thanks a lot.
>
> > --
> > Iñaki Baz Castillo <i...@aliax.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Potential Rack-Contrib contributions
  2010-01-17 18:32           ` Geoff Buesing
@ 2010-01-17 19:15             ` Geoff Buesing
  0 siblings, 0 replies; 9+ messages in thread
From: Geoff Buesing @ 2010-01-17 19:15 UTC (permalink / raw)
  To: Rack Development

... and one more:

http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/9-new-middleware-racktrailingslashredirect


On Jan 17, 12:32 pm, Geoff Buesing <gbues...@gmail.com> wrote:
> @Ryan - patches added as Lighthouse tickets:
>
> http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/3-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/4-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/5-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/6-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/7-n...http://rack.lighthouseapp.com/projects/22456-rack-contrib/tickets/8-r...
>
> Thanks!
>
> Geoff
>
> On Jan 16, 11:47 am, Geoff Buesing <gbues...@gmail.com> wrote:
>
>
>
> > @Ryan - thanks, I'll pull together patches. And I agree that
> > Rackables::Get is to generic of a name; SimpleGetEndpoint is much
> > better. Running with that idea, I'm thinking of going a bit more
> > generic: Rack::SimpleEndpoint, with a verb requirement as an optional
> > argument.
>
> > @Iñaki and @Michael - thanks for the case-sensitivity heads up, and
> > the regex. I'll make sure to put in tests for these cases.
>
> > Geoff
>
> > On Jan 16, 8:47 am, Iñaki Baz Castillo <i...@aliax.net> wrote:
>
> > > El Sábado, 16 de Enero de 2010, Michael Fellinger escribió:
>
> > > > > Well for sure we can assume that other Rack middlewwares and also our
> > > > > Rack app would generate a simple inline header value. Then I would
> > > > > rewrite the regex to this one (the ";" is important):
>
> > > > >  HAS_CHARSET = /;[\s\t]*charset[\s\t]*=[\s\t]*/i
>
> > > > "\t" =~ /\s/
> > > > # 0
> > > > means
> > > > /;\s*charset\s*=\s*/i
> > > > will be enough.
>
> > > Right, my fault, normally I use [ \t]* instead of [\s\t]* which makes no sense
> > > as \s includes space and tabulator.
>
> > > > Regarding the case of Content-Type in the header Hash, as long as it's
> > > > an instance of HeaderHash, the case will not matter, we normalize it
> > > > behind the scene.
>
> > > ok, that's why I said "in case 2 perhaps I'm wrong" :)
>
> > > Thanks a lot.
>
> > > --
> > > Iñaki Baz Castillo <i...@aliax.net>

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2010-01-17 19:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-15 16:23 Potential Rack-Contrib contributions Geoff Buesing
2010-01-16 12:29 ` Ryan Tomayko
2010-01-16 13:22 ` Iñaki Baz Castillo
2010-01-16 13:38   ` Iñaki Baz Castillo
2010-01-16 14:01     ` Michael Fellinger
2010-01-16 14:47       ` Iñaki Baz Castillo
2010-01-16 17:47         ` Geoff Buesing
2010-01-17 18:32           ` Geoff Buesing
2010-01-17 19:15             ` Geoff Buesing

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/rack.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).