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

Convert irc package to js #7022

Merged
merged 13 commits into from
May 31, 2017
Merged

Convert irc package to js #7022

merged 13 commits into from
May 31, 2017

Conversation

MartinSchoeler
Copy link
Contributor

@MartinSchoeler MartinSchoeler commented May 18, 2017

@RocketChat/core

@MartinSchoeler MartinSchoeler changed the title Convert irc package to-js Convert irc package to js May 18, 2017
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 18, 2017 19:18 Inactive
@MartinSchoeler
Copy link
Contributor Author

@ggazzo i will need your help testing this

// message order could not make sure here
this.isConnected = true;
const messageBuf = this.msgBuf;
messageBuf.forEach(msg => {
Copy link
Member

Choose a reason for hiding this comment

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

one line

}

createUserWhenNotExist(name) {
let user;
Copy link
Member

Choose a reason for hiding this comment

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

this logic is seem good, but at least use one line to declare and set the variable.

};

IrcClient.create = function(login) {
let ircClient;
Copy link
Member

Choose a reason for hiding this comment

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

declare where the variable is used. (line 389).

const IRC_AVAILABILITY = RocketChat.settings.get('IRC_Enabled');

// Cache prep
const net = Npm.require('net');
Copy link
Member

Choose a reason for hiding this comment

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

I think that you should use import

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 25, 2017 13:39 Inactive
@MartinSchoeler
Copy link
Contributor Author

@ggazzo fixed the things you mentioned

ggazzo
ggazzo previously requested changes May 29, 2017
return login;
};

class IrcLoginer {
Copy link
Member

Choose a reason for hiding this comment

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

use a function instead class

}
}

class IrcSender {
Copy link
Member

Choose a reason for hiding this comment

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

use a function instead class


}

class IrcRoomJoiner {
Copy link
Member

Choose a reason for hiding this comment

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

use a function instead class


}

class IrcRoomLeaver {
Copy link
Member

Choose a reason for hiding this comment

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

use a function instead class


}

class IrcLogoutCleanUper {
Copy link
Member

Choose a reason for hiding this comment

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

use a function instead class

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 29, 2017 19:17 Inactive
@ggazzo
Copy link
Member

ggazzo commented May 29, 2017

@haosdent @toxicgumbo could you help us to verify if this package still works?

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 31, 2017 13:40 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 31, 2017 14:10 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 31, 2017 14:23 Inactive
Force Travis Restart
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-7022 May 31, 2017 17:07 Inactive
@rodrigok rodrigok merged commit 2f94694 into develop May 31, 2017
@rodrigok rodrigok deleted the irc-to-js branch May 31, 2017 18:00
@rodrigok rodrigok added this to the 0.57.0 milestone May 31, 2017
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

4 participants