Added CacheControl headers #124

nyarly · 2013-10-16T23:37:05Z

Was considering a more flexible API for Response#set_cache_control, but that can wait, seems to me.

ghost · 2013-10-16T23:43:15Z

spec/spec_helper.rb

@@ -5,6 +5,7 @@
 require 'webmachine'
 require 'rspec'
 require 'logger'
+require 'webmachine/adapters/rack'


This seems to be an artifact of local testing, same for the changes to .gitignore and Gemfile. I'm in favor of the latter two, but that should be dealt with in a separate pull request.

Artifact of trying to run a single spec file - spec_helper explicitly uses Webmachine::Adapters::Rack in a before(:suite). Having the require happen accidentally in a spec file seemed like an oversight

Why not run the entire suite? It takes less than 2 seconds for me. 😉

Well, for whatever reason in my env, I get a fail. Also, single file takes <.1sec so...

The point remains: spec_helper makes use of constants that are defined in that file - it should therefore require the file itself. As it stands, the whole suite works properly only because of how rspec loads files (which might change?) as opposed to how Ruby loads files.

I agree with nyarly. We use the Rack adapter in spec_helper.rb, we should require it in spec_helper.rb. What confuses me is why we use the Rack adapter in spec_helper.rb.

Ah right I didn't see that when I made the remark about this line. @samwgoldman it should be rather easy to move these adapter references away, think config.let(:options) { ... } and using that in the adapter specs. Different topic/PR though.

Also agree that it's useful being able to run individual spec files. When running the whole suite, the overhead is not just from the rest of the suite, but also from booting Rake (in the case of bundle exec rake). You also get to enjoy RSpec's CLI options. But again, different topic, and should be dealt with in another PR, for the sake of this PR's clarity.

Let's just make it consistent and clean, however that works. I'm of little opinion on the matter. 😄

ghost · 2013-10-17T00:37:04Z

Very cool work, can't wait to give it a try on master!

nyarly · 2013-10-17T00:37:13Z

Any more comment? I think I've got clear through lines on everything.

ghost · 2013-10-17T00:41:46Z

That was everything from me 🚢 🌟

ghost · 2013-10-17T01:03:14Z

lib/webmachine/cache_control.rb

+          directives[:extensions].merge!( name => parsed_value )
+        end
+      end
+      cache_control = self.new(directives)


this method is really big. can it be condensed into smaller responsibilities? (even with private methods)

Since it's a class method, I'd lean towards composing into a separate class and have ::parse instantiate that... Seems a little heavyweight unless it's going to get other use parsing headers.

fair...
I guess I see two parts in this method. strings.each and down might be better as its own method.

heavyweight in what way? i'd be fine with a class too, but i think a method is fine also.
it might make it easier to reason about, and possibly even test, because you can test each part individually if you want, which can let you get more specific in some tests. I don't think a class is needed, but that'd be fine too for me. parsing isn't usually something that fits into one method.

Heh - I'm used to the god method being a giant switch parser 😄

As I've been working with this today, I'm thinking that the best refactor would be to create per-directive objects, classed based on their values - parse, render, validate all on those. Which thought led to the "is there a HTTP header value parser" question.

ghost · 2013-10-17T01:11:20Z

looks good, +1

seancribbs · 2013-10-17T08:23:14Z

Could we get some more documentation on this? Install yard if you haven't already and check that the doc % doesn't go down after your patch. I think the command is yard --stats, but I don't recall exactly.

seancribbs · 2013-10-17T08:23:36Z

Gemfile

@@ -16,6 +16,10 @@ group :webservers do
  gem 'hatetepe', '~> 0.5.2'
 end

+group :test do
+  gem "fuubar"


What is this dependency?

That's my local testing setup. I'll pull that out. Nice rspec formatter though - progress bar except for failing specs.

Indeed, using it as well, it's sweet.

nyarly · 2013-10-17T19:03:58Z

I've got yard installed - I'll need to add some docs for sure.

Should yard be in the Gemfile, though?

ghost · 2013-10-17T19:06:11Z

it's a development/developer dependency, i think it should be.

nyarly · 2013-10-17T19:07:24Z

As I'm working though this, I'm also wondering: is there not a ready-made HTTP Header parse/generate gem? If so, is it a good idea to be adding a bunch of code here rather than depending on that?

ghost · 2013-10-17T19:10:46Z

i haven't used it, don't know how well it will apply to what we're doing here, but i've seen a few projects use https://github.com/tmm1/http_parser.rb

ghost · 2013-10-17T19:16:27Z

https://github.com/tmm1/http_parser.rb

Will parse entire HTTP messages, but not the individual header fields, i.e. you'll get a hash like { 'Cache-Control': 'no-store foo bar' }.

ghost · 2013-10-17T19:19:20Z

might be useful to use in Webmachine::Header.

nyarly · 2013-10-17T19:19:29Z

Yeah - I'm thinking the value parsing - maybe there's not that many HTTP headers with complex values? Authorization comes to mind as needing parsing.

nyarly · 2013-10-17T19:58:31Z

Doc stats at branch point: 87%. This PR definitely hurts that - working on that now.

nyarly · 2013-10-17T21:18:26Z

Back up to 87%. I believe I've addressed all comments

Ready to merge? Or did I miss something?

ghost · 2013-10-18T03:25:27Z

lib/webmachine/cache_control.rb

+    #
+    #@return [true|false]
+    def valid?
+      validate!


is there any reason to not implement validate! as a private method and return true/false from there?
seems strange to implement valid? on top of a rescue from a method that could return true/false instead.
is there a use case for the raise(validate!) method?

ghost · 2013-10-18T03:25:57Z

@nyarly looks good to me.
+1

ghost · 2013-10-18T03:27:49Z

lib/webmachine/cache_control.rb

+          directives[:extensions].merge!( name => parsed_value )
+        end
+      end
+      return self.new(directives)


i think this method actually bigger, but i won't hold that against you :D

I don't think I added to it :)

ghost · 2013-10-20T23:19:05Z

should we be worried about the failure on jruby?
@seancribbs @lgierth seems good for merge?

ghost · 2013-10-20T23:20:12Z

lib/webmachine/cache_control.rb

+    #   )
+    def initialize(hash=nil)
+      @directives = {}
+      unless hash.nil?


hash = {}. update_from_hash(hash). it'd be a no-op and remove the special-casing here.

Any time you use an object (including {}) as a default value for an argument, the object is instantiated on every call to that method. So using nil here reduces allocation somewhat.

I also find that uniformly using nil as a default value has some benefits in terms passing arguments down the chain - you don't have to remember what the default value is if you want to mirror arguments down.

I don't see that argument as having a strong basis, but happy to subside on this one.

I don't see any reason to guard this, one could simply call update_from_hash(hash || {}).

nyarly · 2013-10-21T19:04:06Z

Cursory look makes it seem like the jruby and 1.9.3 fails have to do with Adapters - does Reel work in those environments?

seancribbs · 2013-10-21T22:12:35Z

Have you updated your bundle or local branch? Those both pass for me.

Sean Cribbs

On Oct 21, 2013, at 8:04 PM, Judson Lester [email protected] wrote:

Cursory look makes it seem like the jruby and 1.9.3 fails have to do with Adapters - does Reel work in those environments?

—
Reply to this email directly or view it on GitHub.

nyarly · 2013-10-21T22:13:40Z

Was responding to @robgleeson about Travis. They pass for me as well - I just have the random single fail. But that's on the current master as well.

nyarly · 2013-10-23T18:41:26Z

Merge? Or something else needs fixing?

ghost · 2013-10-23T19:16:56Z

I think we can disregard the build failure as unrelated, and handle it in #126. There are a couple of code comments without spacing left, and it'd be good to squash the commits into one. 👍 from my side then, good work!

nyarly · 2013-10-24T20:21:31Z

Finished spaces in comments. How strong is the feeling that the commits need to be squashed? (It'll take me a bit since it's a process I don't normally use)

nyarly · 2013-10-24T20:32:03Z

Nevermind, that was much easier than I thought it'd be.

ghost · 2013-10-26T04:32:23Z

lib/webmachine/cache_control.rb

+        raise ArgumentError, "Invalid Cache Control directive value: #{name.inspect} can only be 'true' not #{value.inspect}"
+      end
+      if not (ALL_DIRECTIVES.has_key?(name) or
+              [:cache_extension, :extension, :extensions, :cache_extensions].include?(name))


we could cache this array in a constant.
can we wrap the has_key?/include? calls in a descriptive method name?

maybe, directive?(), and extension?().

👍 @robgleeson

ghost · 2013-10-26T04:43:42Z

@nyarly I merged your other PR and it brought conflicts into this one. should be easy to fix, but FYI.

ghost · 2013-10-27T12:15:34Z

@robgleeson I'd vote for addressing these in a future PR and merging this one with the conflicts resolved, I'm seeing us run down the rabbit-hole.

ghost · 2013-10-27T16:30:12Z

@lgierth +1

seancribbs · 2013-10-27T17:00:03Z

lib/webmachine/cache_control.rb

+      # Parse the value of a Cache-Control header into a CacheControl object
+      # @param [String] string The string-format header value
+      # @param [Array|nil] expected_extension_tokens Any tokens expected in a
+      # Cache Control header


Indent this line by 2-4 spaces from the comment hash or it will appear on its own in the docs and look strange.

nyarly · 2013-10-27T18:52:21Z

I have to say, I'm agreeing with @lgierth that this is starting to feel like a rabbit hole. It's clear that there are strong feelings about how it should be integrated with the rest of Webmachine, but some of those motivations are opaque to me. So: this patch is available to merge and improve as-is or not.

If not, and I need the functionality in my client app, I can clone the code there and make it fit that project.

ghost · 2013-10-27T18:57:13Z

@nyarly
yeah the turnaround on this PR has been slow. I think a followup PR works well.
I have very strong concerns about set_cache_control. could you address my comments? I think we may have overlooked its implementation.

the concerns may seem opaque but some of the code does not fit well with ruby idioms, it's not a big deal, it can be addressed later.

ghost · 2013-10-27T18:58:29Z

the implementation of set_cache_control does not validate a string (at all). this is not opaque.

nyarly · 2013-10-27T20:14:38Z

I can see fixing up the issue on set_cache_control. Something like:

coerce all values to a CacheControl
either raise an error, or return vacantly if the cc is empty (the latter on the grounds that CacheControl: is equivalent to not adding the CC header)
otherwise, set the value of the header to cc.to_s

Regarding the to_i in to_s: validating the well-formedness of every directive value is definitely past the limit of the amount of work I had in mind for this PR.

ghost · 2013-10-27T20:30:19Z

@nyarly
thats fine, lets address set_cache_control & iterate from there in other PRs.
I can help address some of the comments in this PR.

This PR adds a CacheControl object that aids in the manipulation of HTTP CacheControl headers, and a method Response#set_cache_control to make it easy to use. CacheControl::parse takes a list of acceptable "tokens" which will be parsed as symbols if they were provided as unquoted tokens.

nyarly · 2014-01-08T23:32:54Z

So, can I merge this now?

ghost · 2014-01-08T23:34:57Z

if github doesn't go down…
it's been a long time since I went through this code, I think there's still some outstanding issues. I'm not sure if #124 (comment) has happened or not.

bethesque · 2015-01-15T08:13:26Z

Year old PR, can we close this if nobody wants it/is working on it? (We can always reopen it if somebody wants it.)

Asmod4n · 2016-04-24T08:29:56Z

Sorry @nyarly for the long wait here. I know how this must feel, but i am closing it now.
Could you extract your ideas into a separate gem?

What webmachine currently lacks is a stable way to extend it i believe.
Rack is built around the idea to the extensible but i for one completely dislike it how its implemented.
I believe we can find a better way here in webmachine.

@seancribbs @bethesque @lgierth @samwgoldman

nyarly · 2016-04-24T21:08:23Z

I appreciate the sympathy. It's been some time since I used ruby-webmachine, to be honest - I remember I was a little irritated at the time... If I come back to webmachine (in any incarnation) I'll look into it again.

ghost reviewed Oct 16, 2013
View reviewed changes

ghost reviewed Oct 17, 2013
View reviewed changes

seancribbs reviewed Oct 17, 2013
View reviewed changes

ghost reviewed Oct 18, 2013
View reviewed changes

ghost reviewed Oct 20, 2013
View reviewed changes

ghost reviewed Oct 26, 2013
View reviewed changes

seancribbs reviewed Oct 27, 2013
View reviewed changes

ghost mentioned this pull request Dec 3, 2013

Cache-Control header #122

Closed

Asmod4n closed this Apr 24, 2016

Added CacheControl headers #124

Added CacheControl headers #124

Conversation

nyarly commented Oct 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 17, 2013

nyarly commented Oct 17, 2013

ghost commented Oct 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 17, 2013

seancribbs commented Oct 17, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyarly commented Oct 17, 2013

ghost commented Oct 17, 2013

nyarly commented Oct 17, 2013

ghost commented Oct 17, 2013

ghost commented Oct 17, 2013

ghost commented Oct 17, 2013

nyarly commented Oct 17, 2013

nyarly commented Oct 17, 2013

nyarly commented Oct 17, 2013

Choose a reason for hiding this comment

ghost commented Oct 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyarly commented Oct 21, 2013

seancribbs commented Oct 21, 2013

nyarly commented Oct 21, 2013

nyarly commented Oct 23, 2013

ghost commented Oct 23, 2013

nyarly commented Oct 24, 2013

nyarly commented Oct 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 26, 2013

ghost commented Oct 27, 2013

ghost commented Oct 27, 2013

Choose a reason for hiding this comment

nyarly commented Oct 27, 2013

ghost commented Oct 27, 2013

ghost commented Oct 27, 2013

nyarly commented Oct 27, 2013

ghost commented Oct 27, 2013

nyarly commented Jan 8, 2014

ghost commented Jan 8, 2014

bethesque commented Jan 15, 2015

Asmod4n commented Apr 24, 2016

nyarly commented Apr 24, 2016