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 channel join regression and fix possibly joining parted channels #411

Merged
merged 1 commit into from Jun 19, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 17, 2016

This PR creates channels out of join string when creating a network to avoid reusing this join string on future reconnections.

This PR also fixes a regression which was introduced in 2.0-pre: #358.

@xPaw xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Jun 17, 2016
@xPaw xPaw added this to the 2.0.0 milestone Jun 17, 2016
text: cmd
});
}, delay);
delay += 1000;
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified a bit;

const delay = 1000;
commands.forEach((text, idx) => {
    setTimeout(() => client.input({
          target: network.channels[0].id,
          text,
     }), delay * ++idx);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Joins have to run after commands, so that won't work. I also didn't touch this code, only moved it to another file.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I'm not sure the delay * ++idx statement is something I would call "simplified"...

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with @astorije, plus apart from changing the functions to arrow functions (thus breaking older versions), it doesn't actually change much. idx*1000 would do the same, as forEach already provide the index and thus doesn't need to be incremented.

@maxpoulin64
Copy link
Member

Looks good to me, 👍

channels.push(new Chan({
name: chan
}));
});
Copy link
Member

@astorije astorije Jun 19, 2016

Choose a reason for hiding this comment

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

What about:

channels = args.join.replace(/\,/g, " ").split(/\s+/g).map(function(chan) {
    return new Chan({
        name: chan,
    });
});

or, if you don't want that long first line:

var join = args.join.replace(/\,/g, " ").split(/\s+/g);
channels = join.map(...)

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, got ruled out in priv by @xPaw, who plans to use the channels variable in a further PR and cannot blindly overwrite it.

@astorije
Copy link
Member

👍

@astorije astorije merged commit e8cc465 into master Jun 19, 2016
@astorije astorije deleted the xpaw/join branch June 19, 2016 16:59
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Fix channel join regression and fix possibly joining parted channels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants