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

Define which message types should not be logged #2022

Merged
merged 1 commit into from Feb 2, 2018

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jan 31, 2018

whois log is currently logged as undefined whois which isn't at all useful.

@xPaw xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Jan 31, 2018
@AlMcKinlay AlMcKinlay added this to the 2.7.1 milestone Jan 31, 2018
Msg.NonLoggableTypes = {};
Msg.NonLoggableTypes[Msg.Type.BANLIST] = true;
Msg.NonLoggableTypes[Msg.Type.MOTD] = true;
Msg.NonLoggableTypes[Msg.Type.WHOIS] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Msg.NonLoggableTypes = { [Msg.Type.BANLIST]: true, ...}

maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't work on node 4 afaik.

Copy link
Member

Choose a reason for hiding this comment

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

$ nvm use 4
Now using node v4.8.4 (npm v2.15.11)
$ node
> a = 42
42
> { [a]: 1}
{ '42': 1 }

🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Why not add a method to the Msg class instead that would do:

function isLoggable() {
	return this.type !== Msg.Type.MOTD &&
		this.type !== Msg.Type.BANLIST &&
		this.type !== Msg.Type.WHOIS;
}

And in writeUserLog:

if (!msg.isLoggable()) {
	return false;
}

It seems more idiomatic to me, no?
(I went for isLoggable and not isNotLoggable because it's fairly common practice to avoid negated boolean functions)

Bonus, you can get rid of const Msg = require("./msg"); 😁

Msg.NonLoggableTypes = {};
Msg.NonLoggableTypes[Msg.Type.BANLIST] = true;
Msg.NonLoggableTypes[Msg.Type.MOTD] = true;
Msg.NonLoggableTypes[Msg.Type.WHOIS] = true;
Copy link
Member

Choose a reason for hiding this comment

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

$ nvm use 4
Now using node v4.8.4 (npm v2.15.11)
$ node
> a = 42
42
> { [a]: 1}
{ '42': 1 }

🤷‍♂️

Msg.NonLoggableTypes = {};
Msg.NonLoggableTypes[Msg.Type.BANLIST] = true;
Msg.NonLoggableTypes[Msg.Type.MOTD] = true;
Msg.NonLoggableTypes[Msg.Type.WHOIS] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why not add a method to the Msg class instead that would do:

function isLoggable() {
	return this.type !== Msg.Type.MOTD &&
		this.type !== Msg.Type.BANLIST &&
		this.type !== Msg.Type.WHOIS;
}

And in writeUserLog:

if (!msg.isLoggable()) {
	return false;
}

It seems more idiomatic to me, no?
(I went for isLoggable and not isNotLoggable because it's fairly common practice to avoid negated boolean functions)

Bonus, you can get rid of const Msg = require("./msg"); 😁

@xPaw
Copy link
Member Author

xPaw commented Feb 1, 2018

Changed it to what @astorije suggested.

@xPaw xPaw requested a review from astorije February 1, 2018 08:07
Copy link
Member

@AlMcKinlay AlMcKinlay left a comment

Choose a reason for hiding this comment

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

Yes, much nicer.

@astorije astorije merged commit ec70bd9 into master Feb 2, 2018
@astorije astorije deleted the xpaw/not-loggable-types branch February 2, 2018 05:47
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