Add dev caching toggle / server options #20961

Merged
merged 1 commit into from Aug 5, 2015

Conversation

Projects
None yet
3 participants
@ccallebs
Contributor

ccallebs commented Jul 21, 2015

Taken extensively from @Sonopa's PR (#19091). Added ability to specify caching on server start.

Fixes #18875.

@ccallebs

View changes

railties/test/commands/server_test.rb
+ end
+
+ def test_caching_with_option
+ args = ["-C"]

This comment has been minimized.

@ccallebs

ccallebs Jul 21, 2015

Contributor

This could potentially cause some confusion, as there is already a -c option. I think it should be changed, I just didn't have a notion of what it should be.

@ccallebs

ccallebs Jul 21, 2015

Contributor

This could potentially cause some confusion, as there is already a -c option. I think it should be changed, I just didn't have a notion of what it should be.

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Would be easier to grasp as just --toggle-caching - maybe we don't even need a shorthand?

Though I'm still ¯_(ツ)_/¯ about the server options in general.

@kaspth

kaspth Jul 22, 2015

Member

Would be easier to grasp as just --toggle-caching - maybe we don't even need a shorthand?

Though I'm still ¯_(ツ)_/¯ about the server options in general.

This comment has been minimized.

@ccallebs

ccallebs Jul 22, 2015

Contributor

@kaspth I'm ambivalent about the whole thing as well, just added it as it seemed there was support in the previous PR. I'll go with the hivemind on it.

@ccallebs

ccallebs Jul 22, 2015

Contributor

@kaspth I'm ambivalent about the whole thing as well, just added it as it seemed there was support in the previous PR. I'll go with the hivemind on it.

@kaspth

View changes

railties/test/application/rake/dev_test.rb
+module ApplicationTests
+ module RakeTests
+ class RakeDevTest < ActiveSupport::TestCase
+

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

✂️ this line

@kaspth

kaspth Jul 22, 2015

Member

✂️ this line

@@ -0,0 +1,28 @@
+require 'isolation/abstract_unit'

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Don't you have to include Isolation (or something like that) to make this work?

@kaspth

kaspth Jul 22, 2015

Member

Don't you have to include Isolation (or something like that) to make this work?

This comment has been minimized.

@ccallebs

ccallebs Jul 23, 2015

Contributor

@kaspth It didn't appear so, but I've added it just the same.

@ccallebs

ccallebs Jul 23, 2015

Contributor

@kaspth It didn't appear so, but I've added it just the same.

+ boot_rails
+ end
+
+ def teardown

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Could also do teardown :teardown_app

@kaspth

kaspth Jul 22, 2015

Member

Could also do teardown :teardown_app

This comment has been minimized.

@ccallebs

ccallebs Jul 23, 2015

Contributor

@kaspth Is there a preferred way?

@ccallebs

ccallebs Jul 23, 2015

Contributor

@kaspth Is there a preferred way?

This comment has been minimized.

@kaspth

kaspth Jul 25, 2015

Member

Not really 😄

@kaspth

kaspth Jul 25, 2015

Member

Not really 😄

@kaspth

View changes

railties/test/application/rake/dev_test.rb
+ teardown_app
+ end
+
+ test 'dev:cache creates and deletes file and outputs message' do

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Can we split this into two tests?

@kaspth

kaspth Jul 22, 2015

Member

Can we split this into two tests?

@kaspth

View changes

railties/test/application/rake/dev_test.rb
+ assert File.exist?('tmp/caching-dev.txt')
+ assert_match(/Development mode is now being cached/, output)
+ output = `rake dev:cache`
+ assert !File.exist?('tmp/caching-dev.txt')

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Use assert_not

@kaspth

kaspth Jul 22, 2015

Member

Use assert_not

@@ -0,0 +1,13 @@
+namespace :dev do
+ task :cache do
+ desc 'Toggle development mode caching on/off'

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Add a newline after this.

@kaspth

kaspth Jul 22, 2015

Member

Add a newline after this.

+ else
+ FileUtils.touch 'tmp/caching-dev.txt'
+ puts 'Development mode is now being cached.'
+ end

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Add a newline after this too.

@kaspth

kaspth Jul 22, 2015

Member

Add a newline after this too.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 22, 2015

Member

If you change your pull request description to say "Fixes #18875" the original issue will be closed if this is merged 😄

Member

kaspth commented Jul 22, 2015

If you change your pull request description to say "Fixes #18875" the original issue will be closed if this is merged 😄

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Jul 22, 2015

Contributor

@kaspth Thank you for the suggestions -- I'll implement them this evening!

Contributor

ccallebs commented Jul 22, 2015

@kaspth Thank you for the suggestions -- I'll implement them this evening!

@kaspth

View changes

railties/lib/rails/commands/server.rb
@@ -67,6 +69,7 @@ def start
print_boot_information
trap(:INT) { exit }
create_tmp_directories
+ create_cache_file if options[:cache]

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

It seems like setting --perform-caching once never removes the file and leaves caching on all the time. Let's fix that 😄

@kaspth

kaspth Jul 22, 2015

Member

It seems like setting --perform-caching once never removes the file and leaves caching on all the time. Let's fix that 😄

+ else
+ config.action_controller.perform_caching = false
+ config.cache_store = :null_store
+ end

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

I'm not sure we should have such complex logic in the development environment file. But I don't really have better ideas.

@kaspth

kaspth Jul 22, 2015

Member

I'm not sure we should have such complex logic in the development environment file. But I don't really have better ideas.

This comment has been minimized.

@kaspth

kaspth Jul 22, 2015

Member

Never mind, in the original issue @dhh said he was fine with it, so 👍

@kaspth

kaspth Jul 22, 2015

Member

Never mind, in the original issue @dhh said he was fine with it, so 👍

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Jul 23, 2015

Contributor

@kaspth I implemented most of your suggestions. Instead of having a --toggle-cache option on the server, I chose to remove caching if the option wasn't specified. It made more sense to me that way, as a user will know when they start their server whether or not they would like to temporarily cache.

Contributor

ccallebs commented Jul 23, 2015

@kaspth I implemented most of your suggestions. Instead of having a --toggle-cache option on the server, I chose to remove caching if the option wasn't specified. It made more sense to me that way, as a user will know when they start their server whether or not they would like to temporarily cache.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jul 23, 2015

Member

Awesome. Thanks for working on this!

Member

dhh commented Jul 23, 2015

Awesome. Thanks for working on this!

@kaspth

View changes

railties/CHANGELOG.md
@@ -1,3 +1,11 @@
+* Make enabling or disabling caching in development mode possible with rake dev:cache
+
+ Running rake dev:cache will create or remove tmp/caching-dev.txt. When this file exists config.action_controller.perform_caching will be set to true in config/environments/development.rb.

This comment has been minimized.

@kaspth

kaspth Jul 25, 2015

Member

Break the lines at 80 chars 😄

@kaspth

kaspth Jul 25, 2015

Member

Break the lines at 80 chars 😄

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 25, 2015

Member

Alright, once the final comment has been addressed, ping me. Remember to keep it to one commit, then I'll gladly merge 😁

Member

kaspth commented Jul 25, 2015

Alright, once the final comment has been addressed, ping me. Remember to keep it to one commit, then I'll gladly merge 😁

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 25, 2015

Member

I just realized there's a scenario we have to make sure doesn't happen. Maybe I'm misunderstanding something, but what happens when rake dev:cache reboots the server?

There's no --perform-caching flag provided, so won't the caching be turned off again?

I'd like a test that checks against this.

Member

kaspth commented Jul 25, 2015

I just realized there's a scenario we have to make sure doesn't happen. Maybe I'm misunderstanding something, but what happens when rake dev:cache reboots the server?

There's no --perform-caching flag provided, so won't the caching be turned off again?

I'd like a test that checks against this.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Jul 27, 2015

Contributor

@kaspth That's a good point! I'll write the test and if it fails find a workaround.

Contributor

ccallebs commented Jul 27, 2015

@kaspth That's a good point! I'll write the test and if it fails find a workaround.

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Jul 30, 2015

Contributor

@kaspth The easiest way to fix the above is just by defaulting the server to create the cache file if options are passed. Otherwise, default to the last rake dev:cache command. I've pushed the changes up.

Additionally, I couldn't find any concrete examples of tests that check the server start. Could you point me toward some (if they exist)?

Contributor

ccallebs commented Jul 30, 2015

@kaspth The easiest way to fix the above is just by defaulting the server to create the cache file if options are passed. Otherwise, default to the last rake dev:cache command. I've pushed the changes up.

Additionally, I couldn't find any concrete examples of tests that check the server start. Could you point me toward some (if they exist)?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 30, 2015

Member

Honestly, that --perform-caching setting doesn't make sense anymore. If I passed an option on one server start and then removed it for the next server start I wouldn't expect it to still be in effect.

Looking back to @pixeltrix's original suggestion I see --perform-caching takes an argument, which is exactly how we should do it.

Perhaps --perform-caching=true to enable and --perform-caching=false to disable could do it. There's probably a separate issue with what values can be truthy and falsy on a command line interface, but we can deal with that later.

Member

kaspth commented Jul 30, 2015

Honestly, that --perform-caching setting doesn't make sense anymore. If I passed an option on one server start and then removed it for the next server start I wouldn't expect it to still be in effect.

Looking back to @pixeltrix's original suggestion I see --perform-caching takes an argument, which is exactly how we should do it.

Perhaps --perform-caching=true to enable and --perform-caching=false to disable could do it. There's probably a separate issue with what values can be truthy and falsy on a command line interface, but we can deal with that later.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 30, 2015

Member

@ccallebs Sorry, I'm not aware of any tests checking the server start.

Member

kaspth commented Jul 30, 2015

@ccallebs Sorry, I'm not aware of any tests checking the server start.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 4, 2015

Member

@ccallebs are you up for adding an argument to --perform-caching as mentioned here: #20961 (comment)?

Member

kaspth commented Aug 4, 2015

@ccallebs are you up for adding an argument to --perform-caching as mentioned here: #20961 (comment)?

@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 4, 2015

Contributor

@kaspth Absolutely! Sorry, I've been busy and haven't been able to sit down and do it. I'll try to tackle it this evening.

Contributor

ccallebs commented Aug 4, 2015

@kaspth Absolutely! Sorry, I've been busy and haven't been able to sit down and do it. I'll try to tackle it this evening.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 4, 2015

Member

Cool, glad to hear it 😄

Member

kaspth commented Aug 4, 2015

Cool, glad to hear it 😄

Add rake dev:cache task to enable dev mode caching.
Taken from @Sonopa's commits on PR #19091.

Add support for dev caching via "rails s" flags.

Implement suggestions from @kaspth.

Remove temporary cache file if server does not have flags.

Break at 80 characters in railties/CHANGELOG.md

Remove ability to disable cache based on server options.

Add more comprehensive options: --dev-caching / --no-dev-caching
@ccallebs

This comment has been minimized.

Show comment
Hide comment
@ccallebs

ccallebs Aug 5, 2015

Contributor

@kaspth After looking at the OptionParser documentation, I ended up changing the name of the keyword. (http://docs.ruby-lang.org/en/2.2.0/OptionParser.html)

Now, it would be rails s --dev-caching or rails s --no-dev-caching. It seemed to be the way to define boolean arguments according to that class. If we want to change it to perform-caching=[true|false] I'm completely fine with that, but it looked like this followed OptionParser conventions more closely.

Contributor

ccallebs commented Aug 5, 2015

@kaspth After looking at the OptionParser documentation, I ended up changing the name of the keyword. (http://docs.ruby-lang.org/en/2.2.0/OptionParser.html)

Now, it would be rails s --dev-caching or rails s --no-dev-caching. It seemed to be the way to define boolean arguments according to that class. If we want to change it to perform-caching=[true|false] I'm completely fine with that, but it looked like this followed OptionParser conventions more closely.

kaspth added a commit that referenced this pull request Aug 5, 2015

Merge pull request #20961 from ccallebs/add-dev-mode-caching
Add dev caching toggle / server options

@kaspth kaspth merged commit 2d00aa7 into rails:master Aug 5, 2015

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Aug 5, 2015

Member

@ccallebs Thanks ❤️

Member

kaspth commented Aug 5, 2015

@ccallebs Thanks ❤️

@ccallebs ccallebs deleted the ccallebs:add-dev-mode-caching branch Aug 5, 2015

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