Rantanen / node-mumble

Mumble client in Node.js
MIT License
155 stars 48 forks source link

Broadcast message to channel and sub-channels #116

Closed daveoon closed 5 years ago

daveoon commented 5 years ago

Add a broadcast flag to send text message to sub-channels as well.

codecov-io commented 5 years ago

Codecov Report

Merging #116 into master will decrease coverage by 0.01%. The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   21.03%   21.01%   -0.02%     
==========================================
  Files          12       12              
  Lines        1122     1123       +1     
  Branches      184      185       +1     
==========================================
  Hits          236      236              
- Misses        886      887       +1
Impacted Files Coverage Δ
lib/Channel.js 23.07% <33.33%> (-0.26%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4cd44f1...ae1f5d9. Read the comment docs.

Rantanen commented 5 years ago

What are you doing, Codacy?! D:

I'll need to look at that at some point. At least the curly bits are definitely not intended.

On the other hand, any chance you could split the if/elses on different lines?

if( broadcast )
    this.client.sendMessage(...);
else
    this.client.sendMessage(...);

... just something I'm too used to. ._.

daveoon commented 5 years ago

What are you doing, Codacy?! D:

I'll need to look at that at some point. At least the curly bits are definitely not intended.

On the other hand, any chance you could split the if/elses on different lines?

if( broadcast )
    this.client.sendMessage(...);
else
    this.client.sendMessage(...);

... just something I'm too used to. ._.

Do you need to me change to suit your standard? I can do this and resend the pull request

Rantanen commented 5 years ago

Oh, yeah. Sorry. That was aimed at you.

On the other hand, it seems there's a pile of eslint issues around anyway, so once I get home from the office today I'll probably just merge this in and do some style changes anyway. So if you find time to do that change in the next few hours, go for it - but it's okay if you don't!

Rantanen commented 5 years ago

Merged! Thanks for this!

Also noted that the new parameter was missing documentation, but took care of that together with the style changes.

The change should be out in Version 0.3.19