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.

Sign up for GitHub

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

Jump to bottom

Fill in prefixLookup on network initialization #647

Merged
merged 1 commit into from
Oct 12, 2016
Merged

Fill in prefixLookup on network initialization #647

merged 1 commit into from
Oct 12, 2016

Conversation

nornagon
Copy link
Contributor

Fixes #644.

I'm not sure if directly reaching into irc-framework/src/networkinfo is a good idea here, but it seems like irc-fw only creates this object when you call connect(), and I don't know of a good way to test that.

@xPaw xPaw self-assigned this Sep 27, 2016
@astorije astorije added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Sep 28, 2016
@astorije
Copy link
Member

I have no idea how that's possible because that file hasn't changed since May, but GitHub says you have conflicts on src/models/network.js. Mind rebasing/fixing conflicts?

xPaw
xPaw requested changes Sep 30, 2016
View reviewed changes
src/models/network.js
@@ -1,5 +1,6 @@
var _ = require("lodash");
var Chan = require("./chan");
var NetworkInfo = require("irc-framework/src/networkinfo");
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of requering files like that. I think you can just access the network object in the connect event.

@nornagon
Copy link
Contributor Author

Fair point. I’m not sure how to write a test that calls connect without
actually connecting, though :/
On Thu, Sep 29, 2016 at 22:09 Pavel Djundik notifications@github.com
wrote:

@xPaw requested changes on this pull request.

In src/models/network.js
#647 (review):

@@ -1,5 +1,6 @@
var _ = require("lodash");
var Chan = require("./chan");
+var NetworkInfo = require("irc-framework/src/networkinfo");

Not a fan of requering files like that. I think you can just access the
network object in the connect event.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#647 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAKjABmB_8me5-6uuQffhW4bOd9skho-ks5qvJmfgaJpZM4KH8o_
.

@xPaw
Copy link
Member

xPaw commented Sep 30, 2016

@nornagon tests are not critical to have (in the current state of things anyway).

@astorije
Copy link
Member

astorije commented Oct 9, 2016

@nornagon, @xPaw, is this PR conflicting with #632? Is it still relevant? Does it need updating?

@xPaw
Copy link
Member

xPaw commented Oct 9, 2016

It doesn't conflict, but it needs updating.

On Sun, 9 Oct 2016, 22:35 Jérémie Astori, notifications@github.com wrote:

@nornagon https://github.com/nornagon, @xPaw https://github.com/xPaw,
is this PR conflicting with #632
#632? Is it still relevant?
Does it need updating?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#647 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAlb06jL0ZTJ-XuQxGkndXv_WoPEyUdHks5qyUISgaJpZM4KH8o_
.

@nornagon
Copy link
Contributor Author

moved code to "socket connected" event, dropped test

astorije
astorije reviewed Oct 10, 2016
View reviewed changes
src/plugins/irc-events/connection.js
@@ -48,6 +48,11 @@ module.exports = function(irc, network) {
});

irc.on("socket connected", function() {
network.prefixLookup = {};
_.each(irc.network.options.PREFIX, function(mode) {
Copy link
Member

Choose a reason for hiding this comment

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

Might be a nitpick, but any reason to use Lodash's .each instead of native forEach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, no reason. fixed.

Copy link
Member

Choose a reason for hiding this comment

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

You fixed it in the wrong place 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gah, haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed for real this time

@astorije astorije added this to the 2.1.0 milestone Oct 10, 2016
@astorije astorije self-assigned this Oct 10, 2016
@astorije astorije merged commit 93c4c14 into thelounge:master Oct 12, 2016
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@astorije
Merge pull request thelounge#647 from nornagon/prefix-lookup
3c13c08 
Fill in prefixLookup on network initialization
@xPaw xPaw unassigned astorije and xPaw Mar 12, 2018
@astorije
Copy link
Member

Hey @nornagon, we have free sticker packs for our contributors now!
If you're interested, go fill the form at https://goo.gl/forms/f5usqAEp5DWoeXC92 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xPaw xPaw xPaw approved these changes

@astorije astorije astorije approved these changes

Assignees
No one assigned
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nornagon @astorije @xPaw