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

Merged
merged 3 commits into from
Apr 1, 2013
Merged

Add Windows growlnotify support #267

merged 3 commits into from
Apr 1, 2013

Conversation

denisdefreyne
Copy link
Member

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.

@denisdefreyne
Copy link
Member Author

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.

@denisdefreyne
Copy link
Member Author

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.

@denisdefreyne
Copy link
Member Author

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
Copy link
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
Copy link
Contributor

agross commented Mar 31, 2013

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

@denisdefreyne
Copy link
Member Author

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

@ghost ghost assigned bobthecow Apr 1, 2013
@denisdefreyne
Copy link
Member Author

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants