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

Fix tmux display_message command arguments build process #17

Merged
merged 1 commit into from
Sep 15, 2015

Conversation

kaosf
Copy link
Contributor

@kaosf kaosf commented Sep 15, 2015

I got an error like following with the configuration of display_on_all_clients: true.

14:04:30 - ERROR - Guard::RSpec failed to achieve its <run_on_modifications>, exception was:
> [#] NoMethodError: undefined method `+' for nil:NilClass
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier/tmux.rb:70:in `block in display_message'
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier/tmux.rb:69:in `each'
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier/tmux.rb:69:in `display_message'
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier/tmux.rb:321:in `_display_message'
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier/tmux.rb:246:in `_perform_notify'
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier/base.rb:72:in `notify'
> [#] /somewhere/vendor/bundle/ruby/2.2.0/gems/notiffany-0.0.7/lib/notiffany/notifier.rb:183:in `block in notify'
...

args hasn't been initialized there and don't have to append options continuously so I fixed from += to =.

And more, I think clients has only String elements so removed also if client.

@kaosf
Copy link
Contributor Author

kaosf commented Sep 15, 2015

Ooops, if client shouldn't be removed. It makes an error when display_on_all_clients: false. I reverted the changeset.

@e2
Copy link
Contributor

e2 commented Sep 15, 2015

Could you try adding a spec for this? I'll be glad to help you work things out.

Here are the tests for the TMux::Client with the method:

Here's where to add it and what it would be like:

      ###### In this section of Tmux::Client tests  ...

      describe "#display" do
        it "displays text in given area" do
          expect(sheller).to receive(:run).with("tmux display 'foo'")
          subject.display_message("foo")
        end

        ###### Add something like this  ...

        context "when displaying on all clients" do
          subject { described_class.new(:all) }

          it "displays on every client" do
            allow(sheller).to receive(:stdout).
              with("tmux list-clients -F '\#{client_tty}'") do
              "/dev/ttys001\n"
            end

            expect(sheller).to receive(:run).with("tmux display -c /dev/ttys001 'foo'")
            subject.display_message("foo")
          end
        end
      end

Make sure the test fails before you fix with the +=.

@kaosf
Copy link
Contributor Author

kaosf commented Sep 15, 2015

Thanks for your quick response. Okay, I try it.

@kaosf
Copy link
Contributor Author

kaosf commented Sep 15, 2015

At first, I reverted commits and added a test according to the previous given comment example to fail tests.

@kaosf
Copy link
Contributor Author

kaosf commented Sep 15, 2015

Next, I pushed the changeset to fix this error again to check passing all tests.

@kaosf
Copy link
Contributor Author

kaosf commented Sep 15, 2015

@e2 If you want to squash these many commits, feel free to say that to me. I'll do it and push again with git push -f.

@e2
Copy link
Contributor

e2 commented Sep 15, 2015

Looks good to me. Just squash it and I'll merge and release a fixed version.

Thanks!

@kaosf kaosf force-pushed the fix-tmux-client-display-message branch from b26169d to 12925b3 Compare September 15, 2015 16:44
@kaosf
Copy link
Contributor Author

kaosf commented Sep 15, 2015

I've squashed and pushed new one commit just now.

You're welcome and thanks for your help 😄

e2 added a commit that referenced this pull request Sep 15, 2015
Fix tmux display_message command arguments build process
@e2 e2 merged commit e3e6a6e into guard:master Sep 15, 2015
@e2
Copy link
Contributor

e2 commented Sep 15, 2015

Awesome, thanks!

@e2
Copy link
Contributor

e2 commented Sep 15, 2015

Released as 0.0.8

@kaosf kaosf deleted the fix-tmux-client-display-message branch September 16, 2015 03:08
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.

None yet

2 participants