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

Optionally push system notifications when reloading #113

Merged
merged 1 commit into from
Jun 25, 2014

Conversation

wkrsz
Copy link
Contributor

@wkrsz wkrsz commented Jun 24, 2014

No description provided.

UI.info "Reloading browser: #{paths.join(' ')}"
msg = "Reloading browser: #{paths.join(' ')}"
UI.info msg
Notifier.notify(msg, title: 'Reloading browser', image: :success) if options[:notify]

Choose a reason for hiding this comment

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

Line is too long. [93/80]

@wkrsz
Copy link
Contributor Author

wkrsz commented Jun 24, 2014

@thibaudgg "by default doesn't send notification" spec doesn't seem valuable for me, but since you're measuring code coverage I decided to keep it.

@wkrsz wkrsz mentioned this pull request Jun 24, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-41.88%) when pulling 7e4330e on WojtekKruszewski:notify_option into 7fbe2f3 on guard:master.


describe "#reload_browser(paths = [])" do
it "displays a message" do
expect(Guard::UI).to receive(:info).with("Reloading browser: stylesheets/layout.css stylesheets/style.css")
expect(Guard::UI).to receive(:info).with('Reloading browser: stylesheets/layout.css stylesheets/style.css')

Choose a reason for hiding this comment

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

Line is too long. [113/80]

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling 18d7474 on WojtekKruszewski:notify_option into 7fbe2f3 on guard:master.

end

it "optionally pushes notification" do
expect(::Guard::Notifier).to receive(:notify).with(kind_of(String), have_key(:title))

Choose a reason for hiding this comment

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

Line is too long. [91/80]

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling f528af1 on WojtekKruszewski:notify_option into 7fbe2f3 on guard:master.

@thibaudgg
Copy link
Member

Looks good, please just fix the two houndci issue and it'll be merge-able. Thanks!


it "optionally pushes notification" do
expect(::Guard::Notifier).to receive(:notify)
.with(kind_of(String), have_key(:title))

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling 5bf3ff4 on WojtekKruszewski:notify_option into 7fbe2f3 on guard:master.


it "optionally pushes notification" do
expect(::Guard::Notifier).to receive(:notify)
.with(kind_of(String), have_key(:title))

Choose a reason for hiding this comment

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

Place the . on the previous line, together with the method call receiver.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling c75ea6c on WojtekKruszewski:notify_option into c95da08 on guard:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling c75ea6c on WojtekKruszewski:notify_option into c95da08 on guard:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling 880e632 on WojtekKruszewski:notify_option into c95da08 on guard:master.

@wkrsz
Copy link
Contributor Author

wkrsz commented Jun 25, 2014

@thibaudgg Hound looks happy now, although I personally don't agree 'don\t' do something' is better than "don't do something"

@thibaudgg
Copy link
Member

What about 'does not' ?

@wkrsz
Copy link
Contributor Author

wkrsz commented Jun 25, 2014

areyouawizard

@coveralls
Copy link

Coverage Status

Coverage increased (+0.39%) when pulling 9dc465f on WojtekKruszewski:notify_option into c95da08 on guard:master.

@thibaudgg
Copy link
Member

👌

thibaudgg added a commit that referenced this pull request Jun 25, 2014
Optionally push system notifications when reloading
@thibaudgg thibaudgg merged commit e18f4dd into guard:master Jun 25, 2014
@thibaudgg
Copy link
Member

2.3.0 released, thanks!

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

Successfully merging this pull request may close these issues.

4 participants