radical changes for Dsl, tiny changes to Notifier, changes to most specs, all specs pass except Mac specific ones. #55

anithri · 2011-05-05T09:20:09Z

I don't have a mac so no idea if the mac tests pass. Someone should be able to easily make them work using the linux branch (and non-working mac branch) as an example.

Made slight alteration to Guard::Notifier. pulled out logic into #should_send? to allow for stubbing in tests and added #turn_on to allow more flexibility for when things are or are not sent.

Notifier Specs changed to make pass, expanded and to use new notify strategies. NOTE mac tests not tested.

Guard::Dsl changed massively. overall strategy was to decouple to evaluate_guardfile into "getting the data" and "using the data" parts. this provides the ability to pass a string that contains the contents of a guardfile, or to pass a filename for a guardfile as well as reading the default loc for a guardfile.

Dsl specs changed massivly to support new style of Dsl and make pass

listener/linux_spec changed to add a few :long_running tags and to alter some paths to correct values
listener/polling_spec changed to add a few :long_running tags and to alter some paths to correct values

I'm going to follow this up with code that decouples the locations of script execution and Guardfile from the location of watch directories. currently all the dirs you have to watch must be below the location of the Guardfile. This is is groundwork for #53

…uld_send? to allow for stubbing in tests and added #turn_on to allow more flexibility for when things are or are not sent. Notifier Specs changed to make pass, expanded and to use new notify strategies. NOTE mac tests not tested. Guard::Dsl changed massively. overall strategy was to decouple to evaluate_guardfile into "getting the data" and "using the data" parts. this provides the ability to pass a string that contains the contents of a guardfile, or to pass a filename for a guardfile as well as reading the default loc for a guardfile. Dsl specs changed massivly to support new style of Dsl listener/linux_spec changed to add a few :long_running tags and to alter some paths to correct values listener/polling_spec changed to add a few :long_running tags and to alter some paths to correct values

thibaudgg · 2011-05-06T22:00:21Z

I got this error when launch bundle exec guard on Guard code:

Using Guardfile in current dir
/Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/gems/thor-0.14.6/lib/thor/core_ext/hash_with_indifferent_access.rb:26:in `[]=': can't modify frozen hash (RuntimeError)
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/gems/thor-0.14.6/lib/thor/core_ext/hash_with_indifferent_access.rb:26:in `[]='
    from /Users/Thibaud/Development/Code/guard/lib/guard/dsl.rb:26:in `read_guardfile'
    from /Users/Thibaud/Development/Code/guard/lib/guard/dsl.rb:51:in `prep_guardfile_contents'
    from /Users/Thibaud/Development/Code/guard/lib/guard/dsl.rb:8:in `evaluate_guardfile'
    from /Users/Thibaud/Development/Code/guard/lib/guard.rb:29:in `start'
    from /Users/Thibaud/Development/Code/guard/lib/guard/cli.rb:15:in `start'
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/gems/thor-0.14.6/lib/thor/task.rb:22:in `run'
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/gems/thor-0.14.6/lib/thor/invocation.rb:118:in `invoke_task'
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/gems/thor-0.14.6/lib/thor.rb:263:in `dispatch'
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/gems/thor-0.14.6/lib/thor/base.rb:389:in `start'
    from /Users/Thibaud/Development/Code/guard/bin/guard:6:in `<top (required)>'
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/bin/guard:19:in `load'
    from /Users/Thibaud/.rvm/gems/ruby-1.9.2-p136/bin/guard:19:in `<main>'

Notifier specs are working great (I have handled the merge conflict with the recent modifications).

Please can you also update the README to be in sync with your commits.

Thanks for your work!

PS: Maybe separates smaller pull requests would be easier to handle, don't you think?

rymai · 2011-05-06T22:44:35Z

lib/guard/dsl.rb

    class << self
      def evaluate_guardfile(options = {})
+        options.is_a?(Hash) or raise ArgumentError.new("evaluate_guardfile not passed a hash")
+
        @@options = options


Since you modify the options hash at #L26, you should replace this line with @@options = options.dup here.

rymai · 2011-05-06T23:42:45Z

Ok, I've made some comments on some parts of the commit. Also, could you follow the "conventions" of coding already in use in guard? For example, hashes: hash = { :a => "b" }, not hash = {:a=>"b"}. And in general, give your code some air (especially in the specs, blank lines are welcome ;) )!

Anyway, it would be very cool if you could separate this big commit in one for the Notifier and one for the new Guardfile eval feature. We will have to find out why you had to remove "spec/" from here in order to make the specs pass on Linux (since it's the opposite for Mac).

Last but not least, I'm not sure of the usefulness of passing "a string that contains the contents of a guardfile" to guard start. However, I like the possibility to pass the path to a Guardfile!

It would be something like guard start --guardfile ~/Guardfile I guess (maybe we should start to think of a ".guardfilerc" new feature, enabling a user's wide default Guardfile.

Thanks for all Scott, if you can make the few changes I suggested you, it would be awesome! :) Let me know if you need anything from us.

anithri · 2011-05-07T00:23:55Z

Will do better with coding style.
will separate the commit into 2 smaller ones... (here i come help.gihub.com)

2a. I'm pretty sure I have my IDE and testing setup incorrectly. when I rake spec it needs the 'spec/' so I'm going to drop that change and figure out why RubyMine sees things a level lower.

3 re: usefulness of passing a string. I'm going to be using guard in a different context, and when I do, I won't be using the guard binary or CLI. I want to add a programmatic use case to the existing command line use case. And i think giving the programmatic user the ability to control how and where the data for the 'Guardfile' config comes from make it better. I could theoretically pull info from a database, a pipe, and remote network socket. And separating the source of the guardfile from the evaluation step made adding the string passed argument a piece of cake. Not least of the abilities this allows is for the parent to construct a guardfile from multiple files, stitch it together and pass it to the Dsl for evaluation.

For instance. I use a ~/workspace dir to house all of my vcs based project work. rails sites, gems, etc... what if I could just setup a Guardfile in that directory, that (when told to) automatically includes any Guardfiles it finds in all of the first level subdirectories. You could even get meta, watch the top level directory for new directories and automatically add tehm while its running.

Happy to help, planning on banging on this for most of the weekend.

rymai · 2011-05-07T00:32:58Z

Ok, that's more clear now, thanks!! :)

Have a nice weekend! ;)

anithri · 2011-05-07T00:34:16Z

also, while I have done much ruby (and rails) programming, and much perl before that. It's always been as an end-user. This is the first project I've contributed to so I'm still trying to figure out the (in)formalities of contributing. I won't take offense if you tell me I'm doing it wrong, so long as you tell me how to do it right the next time.

Like now. I have no idea if I'm supposed to close this pull request or not. I figure erring on the side of failing to do something is better than doing something wrong.

rymai · 2011-05-07T00:51:57Z

Hey, no problem!

What I would suggest you is to keep this pull request for the "main" feature, i.e. the Dsl changes. Then you create a new branch in your repo for the changes in Notifier (with your branch rebased on guard:master) that you will be able to pull-request against guard:master.

Does it seems right to you? I think it will be ok this way.

Cheers!

…luate_guardfile into "getting the data" and "using the data" parts. this provides the ability to pass a string that contains the contents of a guardfile, or to pass a filename for a guardfile as well as reading the default loc for a guardfile. Dsl specs changed massivly to support new style of Dsl

@disable

…uld_send? to allow for stubbing in tests and added #turn_on to allow more flexibility for when things are or are not sent. Notifier Specs changed to make pass, expanded and to use new notify strategies. NOTE mac tests not tested. i like the @enable as opposed to @disable, should be easy to reverse if necessary though

anithri · 2011-05-07T07:00:59Z

ugh, i hope that's something you can use. i thought i had pulled it off, but there are the 3 follow up commits showing up there and I don't understand why. I think you can skip the ones you want to skip with that

… was passed to it. Except the options hash is actually a Thor::CoreExt::HashWithIndifferentAccess so, i reorganized the internal access and storage to read original data from the passed hash, but stores it into a local hash. No tests changed since no external behavior changed. All tests passed. This fixes the issue when the binary is run and results in a can't modify frozen hash (RuntimeError)

… was passed to it. Except the options hash is actually a Thor::CoreExt::HashWithIndifferentAccess so, i reorganized the internal access and storage to read original data from the passed hash, but stores it into a local hash. No tests changed since no external behavior changed. All tests passed. This fixes the issue when the binary is run and results in a can't modify frozen hash (RuntimeError) small change to fix specs to run and always have @@orig_options be locked.

Conflicts: lib/guard/notifier.rb spec/guard/notifier_spec.rb

rymai · 2011-05-27T16:04:08Z

Hi, I've merged your changes and improved some things...

Thanks, please try it out and keep pull-requesting against master if you need new features! ;)

rymai reviewed May 6, 2011
View reviewed changes

anithri added 4 commits May 6, 2011 20:53

update to current upstream

526d769

slight changes to specs

c749a31

anithri added 3 commits May 7, 2011 12:04

Merge remote-tracking branch 'upstream/master'

3f15bbc

Conflicts: lib/guard/notifier.rb spec/guard/notifier_spec.rb

rymai merged commit 3f15bbc into guard:master May 27, 2011

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

radical changes for Dsl, tiny changes to Notifier, changes to most specs, all specs pass except Mac specific ones. #55

radical changes for Dsl, tiny changes to Notifier, changes to most specs, all specs pass except Mac specific ones. #55

anithri commented May 5, 2011

thibaudgg commented May 6, 2011

rymai May 6, 2011

rymai commented May 6, 2011

anithri commented May 7, 2011

rymai commented May 7, 2011

anithri commented May 7, 2011

rymai commented May 7, 2011

anithri commented May 7, 2011

rymai commented May 27, 2011

radical changes for Dsl, tiny changes to Notifier, changes to most specs, all specs pass except Mac specific ones. #55

radical changes for Dsl, tiny changes to Notifier, changes to most specs, all specs pass except Mac specific ones. #55

Conversation

anithri commented May 5, 2011

thibaudgg commented May 6, 2011

rymai May 6, 2011

Choose a reason for hiding this comment

rymai commented May 6, 2011

anithri commented May 7, 2011

rymai commented May 7, 2011

anithri commented May 7, 2011

rymai commented May 7, 2011

anithri commented May 7, 2011

rymai commented May 27, 2011