drewdeponte / sublime_guard

Sublime Text 2 Guard Plugin - helps create a smoother development workflow.
MIT License
175 stars 15 forks source link

fix issue where bundle exec guard may not be executed properly #58

Closed ghost closed 12 years ago

ghost commented 12 years ago

run_guard checked for Gemfile before entering project directory, and falled back to run "guard" instead of "bundle exec guard"

ghost commented 12 years ago

Had some issues with this, guard not picked up terminal notifications, tried to use GNTP, so I noticed that "guard" is executed instead of "bundle exec guard". This should fix the issue -- change directory before looking for Gemfile, so Gemfile will be found when exists in project folder.

drewdeponte commented 12 years ago

This looks pretty good. However, there is one that I require before I will pull this in. You have changed the code to no longer be logically equivalent. Let me explain. The original code was the following:

cd "$1" && $cmd

The above code would first attempt to change into the specified project path directory and ONLY if it succeeded would it attempt to run the appropriate command. The code currently in this pull request looks like the following:

function run_guard() {
  cmd="guard"
  cd "$1"
  if [[ -s "Gemfile" ]] ; then
    cmd="bundle exec guard"
  fi
  printf "Running '$cmd'. All output/failures from this point on is from the '$cmd' command.\n\n"
  $cmd
}

The problem with the above code is that even if the change directory fails it is still going to attempt to run $cmd and it should NOT because you don't know what that could be doing or where pwd would really be pointing.

My suggestion would be do change it to something like the following:

function run_guard() {
  cd "$1"
  if [[ $? -ne 0 ]]; then
    cmd="guard"
    if [[ -s "Gemfile" ]] ; then
      cmd="bundle exec guard"
    fi
    printf "Running '$cmd'. All output/failures from this point on is from the '$cmd' command.\n\n"
    $cmd
  else
    printf "Failed to change into project root directory: $1, please correct issue to get guard to run.\n\n"
  fi
}

Please make a change based on my recommendation above and push to your branch again to automatically update the pull request. Also, please test it and don't just copy and paste the code I wrote above because I just wrote it off the cuff and there very well could be syntax errors or logic errors.

Once the above changes have been made and the pull request has been updated I will merge the changes into the mainline for the project.

Thank you for the contribution. This project wouldn't be where it is without awesome people that are willing to spend their time to help maintain and improve Sublime Guard.

drewdeponte commented 12 years ago

@aquator also as a side note for future pull requests. I noticed that you made the changes on your forks "master" branch. This is generally not recommended because it makes keeping up to date with the upstream changes hard. Instead the recommended practice is to keep "master" of your fork pristine and simply pull down changes from upstream into your local "master".

So, instead of you immediately making changes and committing them to "master" and making a pull request from your fork's "master" to the main repo's "master". You should have created what is known as a "topic branch" off of "master", made and committed your changes in that branch, push that branch to your fork, and then made a pull request from your fork's "topic branch" to the primary repo's "master" branch.

Again the reason for doing things this way is to make your life easier once I merge your pull request into the primary repos "master" branch. Lets say I merge your pull request in and then you want to add another change in the future. You will then have to pull changes down from the primary repos master into your local master. But, the way you have done things when you do that it will have to merge your local changes with the changes I merged into upstream. Causing your fork's master branch to diverge from the primary repo's "master" branch.

I hope that helps explain things. Please let me know if you have any questions about this or need clarification.

ghost commented 12 years ago

You are right, my shell knowledge is rusty a bit :) However I think you meant -eq 0 instead of -ne 0 at the condition.

Commited, tested, looks okay to me. As an addition, it would be a good idea to somehow make the plugin off state again (I mean the plugin thinks the guard is running, no matter if cd succeed or not at the moment)

Thank you for the branching tutorial, I am pretty new in git world, and it really makes sense, next time I will do that :)

drewdeponte commented 12 years ago

@aquator you are correct I did intend for it to be -eq 0 instead of -ne 0. I am looking at the changes now....

drewdeponte commented 12 years ago

Looks great! I am going to merge it in right now.

Congrats on your first contribution to the Sublime Text 2 Guard Plugin. This project wouldn't exist and improve without people like you willing to take some time out of their life and contribute.

Thanks!!!