.liftoffrc #51

Closed
wants to merge 20 commits into
from

Projects

None yet

3 participants

@mokagio
Contributor
mokagio commented Jan 14, 2014

Here's a first draft of the .liftoffrc feature. See #47.

The file is a simple JSON, the turn_on_all_options method looks for it first on pwd, then on ~, and if it doesn't find it falls back to the default set of settings.

I think JSON is a natural and smart option for this, but other format could be valid as well, YAML maybe?

Some things left to do

  • .liftoffrc validation
  • Nicer output for the user
  • Update README
  • Write in "proper" Ruby

Feedbacks? (Please forgive my Ruby)


Some new tasks after the first code review round.

  • Use parenthesis for methods names.
  • Use instance methods rather than class.
  • Use YAML instead of JSON.
  • Refactor options getter method to override default option with ~ options, the override those with pwd options.
  • Create file with default options
  • .rspec + Rakefile update
  • Restore liftoff -a behaviour, and plug options inspection on just liftoff.

Forgot something!

  • Remove DEFAULT_INDENTATION_LEVEL
@mokagio
Contributor
mokagio commented Jan 14, 2014

Another thing to do is

  • integration with liftoff init. See #50.
@gfontenot
Member

Hey man, thanks for submitting this. Just wanted to let you know that we're not ignoring it. Trying to find time to respond to it properly.

@mokagio
Contributor
mokagio commented Jan 15, 2014

:) no problems and no rush, liftoff works well already anyway 👍

@mokagio
Contributor
mokagio commented Jan 30, 2014

@gfontenot, @21x9 I'd say it's done. Wanna take a look?

It would be nice if some of your thoughtbot Ruby guys could take a look at it as well, to check the write in proper Ruby point.

@mokagio

I've added this verbose = true options for forward compatibility to a --silent mode

@mokagio

I'm not sure if this chain of options = ... if not options is nice or not. I like it because it's all a one liner, but I can see it being a bit misleading as a syntax.

@gfontenot gfontenot and 1 other commented on an outdated diff Jan 31, 2014
@@ -2,3 +2,5 @@ source 'https://rubygems.org'
# Specify your gem's dependencies in liftoff.gemspec
gemspec
+
+gem "rspec"
@gfontenot
gfontenot Jan 31, 2014 Member

This is rendering oddly in GitHub, but you need a newline at the end of this file. I'm also not sure that the spec gem shouldn't be in the development group in the gemspec.

@mokagio
mokagio Jan 31, 2014 Contributor

Yep, I realised I was missing the :test group only later when I set up Travis.

@gfontenot gfontenot commented on the diff Jan 31, 2014
bin/liftoff
@@ -2,7 +2,6 @@
$:.push File.expand_path("../../lib", __FILE__)
require 'liftoff'
-require 'liftoff/launchpad'
@gfontenot
gfontenot Jan 31, 2014 Member

This is a good catch, but probably out of scope for this PR. I'll make this same change myself.

@gfontenot
gfontenot Jan 31, 2014 Member

This change is now on master (e2d702e)

@gfontenot gfontenot and 1 other commented on an outdated diff Jan 31, 2014
@@ -2,7 +2,6 @@
$:.push File.expand_path("../../lib", __FILE__)
require 'liftoff'
-require 'liftoff/launchpad'
DEFAULT_INDENTATION_LEVEL = 4
@gfontenot
gfontenot Jan 31, 2014 Member

We probably don't need this constant anymore, right?

@mokagio
mokagio Jan 31, 2014 Contributor

Thought that as well, I wanted to look into that more before opening an issue.

@gfontenot gfontenot and 1 other commented on an outdated diff Jan 31, 2014
lib/liftoff/launchpad.rb
@@ -95,11 +97,16 @@ def xcode_helper
end
def turn_on_all_options
- %w(git error todo warnings staticanalyzer).each do |option|
- @opts.fetch_option(option.to_sym).value = true
- end
+ options = OptionsHelper::options_from_pwd
+ options = OptionsHelper::options_from_home if not options
@gfontenot
gfontenot Jan 31, 2014 Member

This is often referred to as an "If Surprise", and we generally try to avoid them (see the first item in our style guide). You could actually write this same thing as options ||= OptionsHelper::options_from_home.

@mokagio
mokagio Jan 31, 2014 Contributor

Nice, I knew it was smelling!

@gfontenot gfontenot commented on an outdated diff Jan 31, 2014
lib/liftoff/launchpad.rb
- @opts.fetch_option(:indentation).value ||= DEFAULT_INDENTATION_LEVEL
+ options.each do |option, value|
+ @opts.fetch_option(option.to_sym).value = value
@gfontenot
gfontenot Jan 31, 2014 Member

This actually changes the behavior of this method pretty significantly. Currently, liftoff and liftoff -a do the same thing, but if we add a liftoffrc to the mix, they should probably do different things. I would expect liftoff -a to force everything to be on, while liftoff should just apply my defaults.

@gfontenot gfontenot commented on an outdated diff Jan 31, 2014
lib/liftoff/launchpad.rb
end
+
@gfontenot
gfontenot Jan 31, 2014 Member

No need for this extra newline.

@gfontenot gfontenot commented on an outdated diff Jan 31, 2014
lib/liftoff/options_helper.rb
+
+ def self.options_from_home verbose=true
+ options_from_file ENV['HOME'] + "/.liftoffrc"
+ end
+
+ private
+
+ def self.options_from_file path, verbose=true
+ if File.exists? path
+ puts "Reading liftoff configurations from #{path}\n\n" if verbose
+ options = JSON.parse IO.read path
+ else
+ # maybe show warning in verbose mode?
+ end
+ end
+end
@gfontenot
gfontenot Jan 31, 2014 Member

Same oddly rendered notification about needing a terminating newline.

@gfontenot gfontenot and 1 other commented on an outdated diff Jan 31, 2014
lib/liftoff/options_helper.rb
@@ -0,0 +1,38 @@
+
+class OptionsHelper
+
+ def self.default_options
@gfontenot
gfontenot Jan 31, 2014 Member

I'm not sure why all of these are class methods, but they should probably be instance methods. That's how all of the other classes work.

@mokagio
mokagio Jan 31, 2014 Contributor

I wrote them as class methods because to me they didn't seem to need to need any context, so I didn't see why instantiating an object to do that stuff.

I tried to google the matter of wether or not using class methods, this, and this are interesting, but I'm still not 100% sold...

Can you guys suggest other reading?

Still it's true that GitHelper has all instance methods, so for the sake of consistency I'll fix this.

@gfontenot gfontenot commented on an outdated diff Jan 31, 2014
lib/liftoff/options_helper.rb
@@ -0,0 +1,38 @@
+
+class OptionsHelper
+
+ def self.default_options
+ settings = {
+ :git => true,
+ :error => true,
+ :todo => true,
+ :warnings => true,
+ :staticanalyzer => true,
+ :indentation => 4
+ }
+ end
+
+ def self.filter_valid_options options
@gfontenot
gfontenot Jan 31, 2014 Member

Use parens for arguments.

@gfontenot gfontenot and 1 other commented on an outdated diff Jan 31, 2014
lib/liftoff/options_helper.rb
+ settings = {
+ :git => true,
+ :error => true,
+ :todo => true,
+ :warnings => true,
+ :staticanalyzer => true,
+ :indentation => 4
+ }
+ end
+
+ def self.filter_valid_options options
+ valid_options = default_options.keys
+ options.select { |key, value| (valid_options.include? key or valid_options.include? key.to_sym) }
+ end
+
+ def self.options_from_pwd verbose=true
@gfontenot
gfontenot Jan 31, 2014 Member

Are you using this verbose flag?

@mokagio
mokagio Jan 31, 2014 Contributor

I've added this verbose = true options for forward compatibility to a --silent mode

But yeah, I guess if we design software in forward compatibility mode we'll never end up doing stuff. YAGNI

@gfontenot gfontenot commented on an outdated diff Jan 31, 2014
lib/liftoff/options_helper.rb
+
+ def self.options_from_pwd verbose=true
+ options_from_file Dir.pwd + '/.liftoffrc'
+ end
+
+ def self.options_from_home verbose=true
+ options_from_file ENV['HOME'] + "/.liftoffrc"
+ end
+
+ private
+
+ def self.options_from_file path, verbose=true
+ if File.exists? path
+ puts "Reading liftoff configurations from #{path}\n\n" if verbose
+ options = JSON.parse IO.read path
+ else
@gfontenot
gfontenot Jan 31, 2014 Member

Don't include the else if we aren't doing anything in it.

@gfontenot
Member

Took a spin through this, and it's a great start. A few general notes:

  • Instead of using the conditional assignments for getting the options in Launchpad, why not move that same thing back into OptionsHelper? That way, all you have to do to get the default options is get them from a single place. The Launchpad shouldn't care how the options are generated.
  • OptionsHelper might be better named as DefaultOptions or something similar. As I noted earlier, instead of using class methods, we should be using instance methods.
  • I'm not sold on JSON as the file format for the config file, honestly. I'm almost thinking that YAML will be a lighter weight solution that's easier to expand upon. I can easily see a future where we let users define a default directory structure, and that would be much easier in YAML.
  • I'm wondering if this is going to need to be smarter than it currently is. As implemented here, it falls back on a per-file basis, but it should probably fall back on a per-option basis. If I have 1 project that uses 2 spaces instead of 4, I should be able to have a simple local liftoffrc that defines that key, and then all of the other keys fall back to the global config or the default config.'
  • Stylistically, we're trying to use parens for method arguments everywhere, so if you could match that, it would fit in better.
  • Holy crap this project has a test in it now! We should probably have more of those.

Pumped about how much this is going to improve liftoff, man. Thanks for lending a hand.

@gfontenot
Member

Last thing that I just thought of: Instead of having the defaults written out in code, it would probably be best to have a lib/defaults/liftoffrc file to read from for the defaults.

@mokagio
Contributor
mokagio commented Jan 31, 2014

Thanks for the code review, I learn a new bunch of stuff in like 5 minutes. Awesome!

Last thing that I just thought of: Instead of having the defaults written out in code, it would probably be best to have a lib/defaults/liftoffrc file to read from for the defaults.

Great idea! It could also work as reference for users to look at when rolling out their own settings.

@mokagio
Contributor
mokagio commented Jan 31, 2014

Oh! Also... 👍 👍 for the YAML, it made sense to me to use JSON for simple key-value settings, but YAML is definitely better for describing a directory structure, plus there's less to write.

@gabebw gabebw and 1 other commented on an outdated diff Jan 31, 2014
@@ -1 +1,8 @@
require "bundler/gem_tasks"
+
+desc "Runs the tests for the project"
@gabebw
gabebw Jan 31, 2014 Member

The changes to this file can be like this instead, to avoid calling system:

require "rspec/core/rake_task"

task :default => :spec

If you want color, I'd add --color to a .rspec file in the root of the project.

@mokagio
mokagio Jan 31, 2014 Contributor

Nice! That's awesome thanks!

One question? Would you check-in the .rspec? Or is stuff like that
considered user specific so it shouldn't be added?

Cheers :)

On Friday, January 31, 2014, Gabe Berke-Williams notifications@github.com
wrote:

In Rakefile:

@@ -1 +1,8 @@
require "bundler/gem_tasks"
+
+desc "Runs the tests for the project"

The changes to this file can be like this instead, to avoid calling system
:

require "rspec/core/rake_task"

task :default => :spec

If you want color, I'd add --color to a .rspec file in the root of the
project.

Reply to this email directly or view it on GitHubhttps://github.com/thoughtbot/liftoff/pull/51/files#r9346409
.

Giò

"Be a yardstick of quality. Some people aren't used to an environment
where excellence is expected" S.J.

@gabebw
gabebw Jan 31, 2014 Member

I think that's up to @gfontenot. If he doesn't want to merge it, you can make a ~/.rspec file just on your computer, and RSpec will always check that.

@gabebw gabebw commented on an outdated diff Jan 31, 2014
lib/liftoff/options_helper.rb
+class OptionsHelper
+
+ def self.default_options
+ settings = {
+ :git => true,
+ :error => true,
+ :todo => true,
+ :warnings => true,
+ :staticanalyzer => true,
+ :indentation => 4
+ }
+ end
+
+ def self.filter_valid_options options
+ valid_options = default_options.keys
+ options.select { |key, value| (valid_options.include? key or valid_options.include? key.to_sym) }
@gabebw
gabebw Jan 31, 2014 Member

We avoid using or - we prefer using || since it behaves as expected. or can have surprising behavior, and I never remember when it's OK to use or, so I just use || all the time.

@mokagio mokagio closed this Feb 4, 2014
@mokagio mokagio reopened this Feb 4, 2014
@mokagio mokagio Consisency fixes.
- Removed "if surprise"
- Using || instead of or
- Using parenthesis for methods arguments
- OptionsHelper uses instance methods
52e8e08
@gabebw gabebw commented on an outdated diff Feb 4, 2014
@@ -2,3 +2,7 @@ source 'https://rubygems.org'
# Specify your gem's dependencies in liftoff.gemspec
gemspec
+
+group :test do
@gabebw
gabebw Feb 4, 2014 Member

This should actually be specified in liftoff.gemspec, like this (near the bottom):

gem.add_development_dependency 'rspec'
@mokagio
Contributor
mokagio commented Feb 5, 2014

@gabebw thanks, and to say that it was also written pretty clearly in the Gemfile...

@mokagio mokagio and 1 other commented on an outdated diff Feb 6, 2014
lib/liftoff/options_helper.rb
+ def default_options
+ options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
+ end
+
+ def options_from_pwd
+ options_from_file(Dir.pwd + '/.liftoffrc')
+ end
+
+ def options_from_home
+ options_from_file(ENV['HOME'] + "/.liftoffrc")
+ end
+
+ def user_default_options
+ evaluted_options = default_options
+ evaluted_options = evaluted_options.merge options_from_home
+ evaluted_options = evaluted_options.merge options_from_pwd
@mokagio
mokagio Feb 6, 2014 Contributor

Is merge all right? Or are there better ways to do it?

@gabebw
gabebw Feb 6, 2014 Member

.merge! will merge in-place, so line 19 and 20 could use it like this:

evaluated_options.merge!(options_from_home)
evaluated_options.merge!(options_from_pwd)
@mokagio mokagio commented on an outdated diff Feb 6, 2014
lib/liftoff/options_helper.rb
+ valid_options = default_options.keys
+ options.select { |key, value| (valid_options.include?(key.to_s) || valid_options.include?(key.to_sym)) }
+ end
+
+ private
+
+ def options_from_file(path)
+ if File.exists? path
+ options = YAML.load_file(path)
+ convert_keys_symbols options
+ end
+ end
+
+ def convert_keys_symbols(hash)
+ Hash[hash.map { |key, value| [key.to_sym, value] }]
+ end
@mokagio
mokagio Feb 6, 2014 Contributor

YAML.load_file returns string keys, but launchpad uses symbols, so I'm converting them here. Does it make sense?

@mokagio mokagio commented on an outdated diff Feb 6, 2014
spec/options_helper_spec.rb
+ end
+ end
+
+ describe "#user_default_options" do
+ it "returns a set of options evaluated starting form pwd, falling back to home, falling back to defaults" do
+ options_helper = OptionsHelper.new
+
+ options_helper.stub(:default_options) { { :pasta => 1, :beer => 1, :cheese_cake => 1 } }
+ options_helper.stub(:options_from_home) { { :pasta => 0, :pizza => 2 } }
+ options_helper.stub(:options_from_pwd) { { :beer => 2, :cheese_cake => 2 } }
+ options_helper.stub(:filter_valid_options).with(anything()) { anything() }
+
+ expected_options = { :pasta => 0, :pizza => 2, :beer => 2, :cheese_cake => 2 }
+ options_helper.user_default_options.should eq(expected_options)
+ end
+ end
@mokagio
mokagio Feb 6, 2014 Contributor

I'm always uncertain when stubbing stuff... is this test fine or is it implying too much knowledge of the implementation?

@mokagio mokagio commented on the diff Feb 6, 2014
liftoff.gemspec
@@ -16,6 +16,8 @@ Gem::Specification.new do |gem|
gem.add_dependency 'xcodeproj', '~> 0.14.1'
gem.add_dependency 'highline', '~> 1.6'
+ gem.add_development_dependency 'rspec'
+
@mokagio
mokagio Feb 6, 2014 Contributor

@gabebw you said to put it close to the bottom, but it made sense to me to place them right below the normal dependencies. How is it?

@mokagio
Contributor
mokagio commented Feb 6, 2014

Yo! Got a bit more time this week and did some work done!

Now .liftoffrc uses YAML format, liftoff does a smart options lookup, and liftoff -a works as it did before, setting everything to the default values.

@gfontenot

OptionsHelper might be better named as DefaultOptions or something similar. As I noted earlier, instead of using class methods, we should be using instance methods.

It made sense to me to use that name to be in line with GitHelper and XcodeHelper. Although GitHelper is now FileManager right? And DefaultOptions speaks a bit more of what that class do... So yeah, I'm gonna change it.

I also made some comments regarding some doubts I have.

@gfontenot
Member

Awesome, yeah. The move from GitHelper to FileManager was prompted by me looking at the current naming convention and not being super happy with them. I'll take a look at this in full tomorrow and I'll brainstorm on the naming.

@mokagio
Contributor
mokagio commented Feb 6, 2014

👍 I like DefaultOptions more than OptionsHelper, but I'm still not super happy. Also doing stuff like DefaultOptions.new.default_options is a bit weird...

@gabebw gabebw and 1 other commented on an outdated diff Feb 6, 2014
lib/liftoff/default_options.rb
+class DefaultOptions
+
+ def default_options
+ options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
+ end
+
+ def options_from_pwd
+ options_from_file(Dir.pwd + '/.liftoffrc')
+ end
+
+ def options_from_home
+ options_from_file(ENV['HOME'] + "/.liftoffrc")
+ end
+
+ def user_default_options
+ evaluted_options = default_options
@gabebw
gabebw Feb 6, 2014 Member

evaluted is misspelled - it should be evaluated.

@mokagio
mokagio Feb 7, 2014 Contributor

Good catch, thanks :)

@gfontenot gfontenot commented on the diff Feb 8, 2014
README.md
@@ -34,6 +34,19 @@ Usage: liftoff [options]
-h, --help Display this help message
```
+### Configuration - .liftoffrc
+
+You can use the `.liftoffrc` file to speedup your workflow by defining your favorite settings for liftoff.
@gfontenot
gfontenot Feb 8, 2014 Member

Lets add a note here about ~/.liftoffrc vs ./.liftoffrc and how the precedence/per-key system works.

@gfontenot gfontenot commented on the diff Feb 8, 2014
README.md
@@ -34,6 +34,19 @@ Usage: liftoff [options]
-h, --help Display this help message
```
+### Configuration - .liftoffrc
+
+You can use the `.liftoffrc` file to speedup your workflow by defining your favorite settings for liftoff.
+
+```YAML
+git:
+ true
@gfontenot
gfontenot Feb 8, 2014 Member

These values can be on the same line as the key. We should recommend that people write it this way.

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/defaults/liftoffrc
@@ -0,0 +1,12 @@
+git:
+ true
@gfontenot
gfontenot Feb 8, 2014 Member

Same note here about using the same line.

Also, I think that defaults should be a sibling to lib instead of a subdirectory.

@mokagio
mokagio Feb 9, 2014 Contributor

Cool, didn't know this about YAML format.

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
+ options_from_file(Dir.pwd + '/.liftoffrc')
+ end
+
+ def options_from_home
+ options_from_file(ENV['HOME'] + "/.liftoffrc")
+ end
+
+ def user_default_options
+ evaluated_options = default_options
+ evaluated_options.merge!(options_from_home)
+ evaluated_options.merge!(options_from_pwd)
+
+ filter_valid_options evaluated_options
+ end
+
+ def filter_valid_options(options)
@gfontenot
gfontenot Feb 8, 2014 Member

Do we need to filter? If there are keys in the config, we just wouldn't ever see them, right?

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
@@ -0,0 +1,44 @@
+require 'yaml'
+
+class DefaultOptions
+
+ def default_options
+ options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
@gfontenot
gfontenot Feb 8, 2014 Member

This can be done as File.expand_path('../defaults/liftoffrc', __FILE__) which is a bit cleaner. Obviously you'll need to back out 2 directories once defaults is a top level directory.

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
@@ -0,0 +1,44 @@
+require 'yaml'
+
+class DefaultOptions
@gfontenot
gfontenot Feb 8, 2014 Member

This should be part of the Liftoff module. (I know this wasn't the case for the other classes, just fixed that on master.)

@gfontenot
gfontenot Feb 8, 2014 Member

How about calling this object OptionParser. That feels more like what it's responsibility is, don't you think?

@mokagio
mokagio Feb 9, 2014 Contributor

Yeah, this makes sense now that the command line options have been removed! 👍

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
@@ -0,0 +1,44 @@
+require 'yaml'
+
+class DefaultOptions
+
+ def default_options
+ options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
+ end
+
+ def options_from_pwd
@gfontenot
gfontenot Feb 8, 2014 Member

Lets call this local_options

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
@@ -0,0 +1,44 @@
+require 'yaml'
+
+class DefaultOptions
+
+ def default_options
+ options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
+ end
+
+ def options_from_pwd
+ options_from_file(Dir.pwd + '/.liftoffrc')
+ end
+
+ def options_from_home
@gfontenot
gfontenot Feb 8, 2014 Member

lets call this user_options

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
+
+class DefaultOptions
+
+ def default_options
+ options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
+ end
+
+ def options_from_pwd
+ options_from_file(Dir.pwd + '/.liftoffrc')
+ end
+
+ def options_from_home
+ options_from_file(ENV['HOME'] + "/.liftoffrc")
+ end
+
+ def user_default_options
@gfontenot
gfontenot Feb 8, 2014 Member

How about we rename this to evaluated_options? Then, we should probably move this to a private method, and have a single public method called options that returns a memoized instance variable like so:

def options
  @options ||= evaluated_options
end

That way, we only hit the disk for the options once, instead of every time we need to access them.

@gfontenot gfontenot commented on the diff Feb 8, 2014
lib/liftoff/default_options.rb
+ evaluated_options.merge!(options_from_pwd)
+
+ filter_valid_options evaluated_options
+ end
+
+ def filter_valid_options(options)
+ valid_options = default_options.keys
+ options.select { |key, value| (valid_options.include?(key.to_s) || valid_options.include?(key.to_sym)) }
+ end
+
+ private
+
+ def options_from_file(path)
+ if File.exists? path
+ options = YAML.load_file(path)
+ convert_keys_symbols options
@gfontenot
gfontenot Feb 8, 2014 Member

Use parens here for the argument

@gfontenot
Member

@mokagio I've actually gone ahead and made these minor fixes myself, and all of this is merged in 03b34d9. Thank you for your work on this!

@gfontenot gfontenot closed this Feb 9, 2014
@mokagio
Contributor
mokagio commented Feb 9, 2014

Cool! Awesome stuff!

Thank you guys for coming up with this in the first place! Such a valuable addition to my toolbelt!

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