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

clean this up #211

Closed
wants to merge 1 commit into from
Closed

clean this up #211

wants to merge 1 commit into from

Conversation

mjgeis
Copy link

@mjgeis mjgeis commented Apr 7, 2023

whoever left it this way, think about your actions

Copy link
Contributor

@11k 11k left a comment

Choose a reason for hiding this comment

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

I'd like to expand the scope of this PR a little to address some obvious issues related to the regex and their use.

Also, would you mind merging in master? commandsinfo has since been moved to a different file.

Comment on lines +382 to +392
const banstruct = {
id: 0,
userid: 0,
username: '',
targetuserid: '',
targetusername: '',
ipaddress: '',
reason: '',
starttimestamp: '',
endtimestamp: '',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can do without this constant altogether. Can you just build the object in the method where it's used?

],
]);

const settingsdefault = new Map([
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind building this, errorstrings, and helpstrings using an object? I'd prefer that over an array like this for better readability. You can use Object.entries() so its broken down into key-value pairs like the constructor expects.

const regexslashcmd = /^\/([a-z0-9]+)[\s]?/i;
const regextime = /(\d+(?:\.\d*)?)([a-z]+)?/gi;
const nickmessageregex = /(?=@?)(\w{3,20})/g;
const nickregex = /^[a-zA-Z0-9_]{3,20}$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const nickregex = /^[a-zA-Z0-9_]{3,20}$/;
const nickregex = /^\w{3,20}$/;

Comment on lines +6 to +7
const nsfwregex = /\b(?:NSFW)\b/i;
const nsflregex = /\b(?:NSFL)\b/i;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the non-capturing group is unnecessary for these two expressions. Would you mind removing them?

const regextime = /(\d+(?:\.\d*)?)([a-z]+)?/gi;
const nickmessageregex = /(?=@?)(\w{3,20})/g;
const nickregex = /^[a-zA-Z0-9_]{3,20}$/;
const nsfwnsflregex = /\b(?:NSFL|NSFW)\b/i;
Copy link
Contributor

Choose a reason for hiding this comment

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

This regex and its related logic seems pointless because we already check for nsfw and nsfl separately in ignored(). I think its safe to remove.

@@ -1,3 +1,21 @@
const regexslashcmd = /^\/([a-z0-9]+)[\s]?/i;
const regextime = /(\d+(?:\.\d*)?)([a-z]+)?/gi;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't even aware there's support for decimal notation here. I think we can remove this to simplify things. I'm 100% positive nobody uses it.

Suggested change
const regextime = /(\d+(?:\.\d*)?)([a-z]+)?/gi;
const regextime = /(\d+)([a-z]+)?/gi;

There's also the issue of commands that accept a time string succeeding when it's incorrectly formatted, but that's something we'll address in a future PR.

@11k 11k added the enhancement New feature or request label Apr 14, 2023
@11k
Copy link
Contributor

11k commented Apr 24, 2023

Just following up to see if you're interested in working through the issues I pointed out. I realize it could be more than you wanted to do. If you don't have the time, let me know and I'll have someone else work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants