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

Add guard support #767

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add guard support #767

wants to merge 3 commits into from

Conversation

aspiers
Copy link
Member

@aspiers aspiers commented Nov 4, 2016

It's indispensable for TDD coding.

@aspiers aspiers force-pushed the guard branch 2 times, most recently from 61f3a5c to 0cd1975 Compare November 4, 2016 23:25
@rsalevsky
Copy link
Member

I would prefer if we integrate it into https://github.com/crowbar/crowbar/blob/master/Guardfile. Then we have all the guard code at one place.

@MaximilianMeister
Copy link
Member

I would prefer if we integrate it into https://github.com/crowbar/crowbar/blob/master/Guardfile. Then we have all the guard code at one place.

but that will not work anymore if you just clone this repo... only works if you set up a development environment with the barclamps folder that has this repo here, which not everybody does.

guard :rspec, guard_opts do
watch('app/controllers/application_controller.rb') {
"spec/controllers"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we mention the application_controller here explicitely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because if the application_controller changes, all controllers need to be retested.

@aspiers
Copy link
Member Author

aspiers commented Nov 7, 2016

@rsalevsky I would prefer the opposite, because they have no goals or code in common.

@rsalevsky
Copy link
Member

There are two reasons why I think it is better if we do it in the crowbar repo:

  1. We save the duplicated code for the gem definition.
  2. You just need to run one command to sync to the remote and see the results. @MaximilianMeister I think if you do crowbar development then you should use also the dev setup. Otherwise I'm not sure how the code should be tested.

If you get another +1 feel free to merge it I don't want to block merging this if it helps making faster progress.

@aspiers
Copy link
Member Author

aspiers commented Nov 8, 2016

We save the duplicated code for the gem definition.

I agree that's a benefit, but only a very small one which is outweighed by the other considerations below:

You just need to run one command to sync to the remote and see the results.

This is mixing two completely different things. One is syncing to a remote server, which is useful for testing deployed code, and the other is running unit tests directly from within a development environment. Sometimes you might want to do one, sometimes the other, and sometimes both at the same time. So it doesn't make sense to couple them together.

Other reasons:

  • It must be possible to update the code and the corresponding Guardfile in a single PR, for example if you add a new subdirectory or filetype which is not covered by the existing patterns. This is pretty important.
  • It should be possible to quickly check out just crowbar-core and hack on it without requiring other repos. This is not so important but still worth having.

Adam Spiers added 3 commits November 24, 2016 21:52
If we do "bundle install --deployment" then gems get installed into
vendor/bundle/ which we don't want interfering with git.
We want the :development group to contain gems which are
needed for interactive development but not for automated testing.
This will allow us to exclude them from CI runs, to speed up the
runs, and also work around that the listen gem isn't supported on
old Rubies any more.
Guard is indispensable for TDD red/green coding.

https://github.com/guard/guard#readme
@aspiers
Copy link
Member Author

aspiers commented Nov 24, 2016

Rebased

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

Successfully merging this pull request may close these issues.

6 participants