Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.liftoffrc #51

Closed
wants to merge 20 commits into from

Conversation

@mokagio
Copy link
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

This comment has been minimized.

Copy link
Contributor Author

mokagio commented Jan 14, 2014

Another thing to do is

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

This comment has been minimized.

Copy link
Collaborator

gfontenot commented Jan 15, 2014

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

This comment has been minimized.

Copy link
Contributor Author

mokagio commented Jan 15, 2014

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

@mokagio

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Owner Author

mokagio commented on lib/liftoff/options_helper.rb in 90dd689 Jan 30, 2014

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

@mokagio

This comment has been minimized.

Copy link
Owner Author

mokagio commented on lib/liftoff/launchpad.rb in 90dd689 Jan 30, 2014

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.

Gemfile Outdated
@@ -2,3 +2,5 @@ source 'https://rubygems.org'

# Specify your gem's dependencies in liftoff.gemspec
gemspec

gem "rspec"

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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.

This comment has been minimized.

Copy link
@mokagio

mokagio Jan 31, 2014

Author Contributor

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

@@ -2,7 +2,6 @@

$:.push File.expand_path("../../lib", __FILE__)
require 'liftoff'
require 'liftoff/launchpad'

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

This change is now on master (e2d702e)

bin/liftoff Outdated
@@ -2,7 +2,6 @@

$:.push File.expand_path("../../lib", __FILE__)
require 'liftoff'
require 'liftoff/launchpad'

DEFAULT_INDENTATION_LEVEL = 4

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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

This comment has been minimized.

Copy link
@mokagio

mokagio Jan 31, 2014

Author Contributor

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

@opts.fetch_option(option.to_sym).value = true
end
options = OptionsHelper::options_from_pwd
options = OptionsHelper::options_from_home if not options

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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.

This comment has been minimized.

Copy link
@mokagio

mokagio Jan 31, 2014

Author Contributor

Nice, I knew it was smelling!


@opts.fetch_option(:indentation).value ||= DEFAULT_INDENTATION_LEVEL
options.each do |option, value|
@opts.fetch_option(option.to_sym).value = value

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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.

end

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

No need for this extra newline.

# maybe show warning in verbose mode?
end
end
end

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

Same oddly rendered notification about needing a terminating newline.


class OptionsHelper

def self.default_options

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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.

This comment has been minimized.

Copy link
@mokagio

mokagio Jan 31, 2014

Author 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.

}
end

def self.filter_valid_options options

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

Use parens for arguments.

options.select { |key, value| (valid_options.include? key or valid_options.include? key.to_sym) }
end

def self.options_from_pwd verbose=true

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

Are you using this verbose flag?

This comment has been minimized.

Copy link
@mokagio

mokagio Jan 31, 2014

Author 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

if File.exists? path
puts "Reading liftoff configurations from #{path}\n\n" if verbose
options = JSON.parse IO.read path
else

This comment has been minimized.

Copy link
@gfontenot

gfontenot Jan 31, 2014

Collaborator

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

@gfontenot

This comment has been minimized.

Copy link
Collaborator

gfontenot commented Jan 31, 2014

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

This comment has been minimized.

Copy link
Collaborator

gfontenot commented Jan 31, 2014

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Contributor Author

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.

Rakefile Outdated
@@ -1 +1,8 @@
require "bundler/gem_tasks"

desc "Runs the tests for the project"

This comment has been minimized.

Copy link
@gabebw

gabebw Jan 31, 2014

Contributor

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.

This comment has been minimized.

Copy link
@mokagio

mokagio Jan 31, 2014

Author 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//pull/51/files#r9346409
.

Giò

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

This comment has been minimized.

Copy link
@gabebw

gabebw Jan 31, 2014

Contributor

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.


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) }

This comment has been minimized.

Copy link
@gabebw

gabebw Jan 31, 2014

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Collaborator

gfontenot commented Feb 6, 2014

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

This comment has been minimized.

Copy link
Contributor Author

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...

end

def user_default_options
evaluted_options = default_options

This comment has been minimized.

Copy link
@gabebw

gabebw Feb 6, 2014

Contributor

evaluted is misspelled - it should be evaluated.

This comment has been minimized.

Copy link
@mokagio

mokagio Feb 7, 2014

Author Contributor

Good catch, thanks :)

@@ -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.

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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


```YAML
git:
true

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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

@@ -0,0 +1,12 @@
git:
true

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

Same note here about using the same line.

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

This comment has been minimized.

Copy link
@mokagio

mokagio Feb 9, 2014

Author Contributor

Cool, didn't know this about YAML format.

filter_valid_options evaluated_options
end

def filter_valid_options(options)

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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

class DefaultOptions

def default_options
options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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.

@@ -0,0 +1,44 @@
require 'yaml'

class DefaultOptions

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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

This comment has been minimized.

Copy link
@mokagio

mokagio Feb 9, 2014

Author Contributor

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

options_from_file( File.join(File.dirname(File.expand_path(__FILE__)), '../defaults/liftoffrc') )
end

def options_from_pwd

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

Lets call this local_options

options_from_file(Dir.pwd + '/.liftoffrc')
end

def options_from_home

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

lets call this user_options

options_from_file(ENV['HOME'] + "/.liftoffrc")
end

def user_default_options

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

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.

def options_from_file(path)
if File.exists? path
options = YAML.load_file(path)
convert_keys_symbols options

This comment has been minimized.

Copy link
@gfontenot

gfontenot Feb 8, 2014

Collaborator

Use parens here for the argument

@gfontenot

This comment has been minimized.

Copy link
Collaborator

gfontenot commented Feb 9, 2014

@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!

@mokagio

This comment has been minimized.

Copy link
Contributor Author

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
Projects
None yet
3 participants
You can’t perform that action at this time.