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 Windows growlnotify support #267

Merged
merged 3 commits into from Apr 1, 2013

Conversation

Projects
None yet
3 participants
@ddfreyne
Member

ddfreyne commented Mar 3, 2013

This fixes #253.

This is a work in progress. There are no tests yet, and I have no clear idea of how I would test this. Perhaps create a stub growlnotify executable and see whether it is called the right way.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Mar 3, 2013

Member

It’s probably also a good idea to move the watcher into a separate object, maybe Nanoc::Extra::Watcher. The notification generation stuff should probably also be extracted, perhaps in Nanoc::Extra::UserNotifier. Easier to test!

Those changes belong in a separate pull request though. Stay tuned.

Member

ddfreyne commented Mar 3, 2013

It’s probably also a good idea to move the watcher into a separate object, maybe Nanoc::Extra::Watcher. The notification generation stuff should probably also be extracted, perhaps in Nanoc::Extra::UserNotifier. Easier to test!

Those changes belong in a separate pull request though. Stay tuned.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Mar 20, 2013

Member

I’ll fix this later in a decent way. I’m refactoring the watcher quite significantly, and this pull request will be obsolete once I’m finished with that.

Member

ddfreyne commented Mar 20, 2013

I’ll fix this later in a decent way. I’m refactoring the watcher quite significantly, and this pull request will be obsolete once I’m finished with that.

@ddfreyne ddfreyne closed this Mar 20, 2013

@ddfreyne ddfreyne reopened this Mar 31, 2013

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Mar 31, 2013

Member

Since the watcher refactoring was cancelled (see #293), I think reopening this pull request makes sense.

I have no clue how to test this automatically. Maybe it isn’t automatically testable. Any ideas? @bobthecow?

@agross Can you test (manually) whether this pull requests works for you? If it does, it could be merged without presence of automated tests.

Member

ddfreyne commented Mar 31, 2013

Since the watcher refactoring was cancelled (see #293), I think reopening this pull request makes sense.

I have no clue how to test this automatically. Maybe it isn’t automatically testable. Any ideas? @bobthecow?

@agross Can you test (manually) whether this pull requests works for you? If it does, it could be merged without presence of automated tests.

@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Mar 31, 2013

Contributor

@ddfreyne I'll test that either tomorrow or on Tuesday.

As for testing, why not check if nanoc builds the command line parameters correctly and calls #system accordingly?

Contributor

agross commented Mar 31, 2013

@ddfreyne I'll test that either tomorrow or on Tuesday.

As for testing, why not check if nanoc builds the command line parameters correctly and calls #system accordingly?

@agross

This comment has been minimized.

Show comment
Hide comment
@agross

agross Mar 31, 2013

Contributor

@ddfreyne I left a comment on ddfreyne/nanoc@43a49c9. If the typo is fixed, it will work. As always, thank you very much!

Contributor

agross commented Mar 31, 2013

@ddfreyne I left a comment on ddfreyne/nanoc@43a49c9. If the typo is fixed, it will work. As always, thank you very much!

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Mar 31, 2013

Member

Awesome. I’ll add a test and release this in 3.6.3!

Member

ddfreyne commented Mar 31, 2013

Awesome. I’ll add a test and release this in 3.6.3!

@ghost ghost assigned bobthecow Apr 1, 2013

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Apr 1, 2013

Member

@bobthecow Review, merge and close #253 afterwards.

Member

ddfreyne commented Apr 1, 2013

@bobthecow Review, merge and close #253 afterwards.

bobthecow added a commit that referenced this pull request Apr 1, 2013

@bobthecow bobthecow merged commit ad4b26e into nanoc:release-3.6.x Apr 1, 2013

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment