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

Mod add command #13

Merged
merged 6 commits into from
Feb 14, 2021
Merged

Mod add command #13

merged 6 commits into from
Feb 14, 2021

Conversation

matanp
Copy link
Owner

@matanp matanp commented Jan 27, 2021

!add {command} {response} {+m for mod only}
-auto counts usage, access with !{command} count (usage persists every 5 minutes, will lose any usages up to 5 minutes prior to turning bot off)
-stores who added it and what day, access with !{command} age

!edit {command} {response} {+m for mod only}

refactored mlk quotes

@matanp matanp requested a review from unfamiliarish2 January 27, 2021 07:40
@matanp matanp linked an issue Jan 27, 2021 that may be closed by this pull request
@@ -0,0 +1 @@
{"commands":[{"command_word":"hello","response_array":["world"],"mod_only":false,"added_by":"matanjuggles","added_date":"1-25, 2021","usage_count":6},{"command_word":"a","response_array":["this","has","been","used","{count}","times"],"mod_only":false,"added_by":"matanjuggles","added_timestamp":"1-28, 2021","usage_count":8}]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would indent this, for readability

Add a newline at the end of the file

Comment on lines 78 to 93
const quotes = [
"I have a dream that one day this nation will rise up and live out the true meaning of its creed: We hold these truths to be self-evident that all men are created equal. I have a dream that my four little children will one day live in a nation where they will not be judged by the color of their skin but by the content of their character. I have a dream today.",
"The ultimate measure of a man is not where he stands in moments of comfort and convenience, but where he stands at times of challenge and controversy.",
"Injustice anywhere is a threat to justice everywhere.",
"The time is always right to do what is right.",
"I have decided to stick with love. Hate is too great a burden to bear.",
"Darkness can not drive out darkness; only light can do that. Hate cannot drive out hate; only love can do that.",
"Nothing in the world is more dangerous than sincere ignorance and conscientious stupidity.",
"There can be no deep disappointment where there is not deep love.",
"I have a dream that one day this nation will rise up and live out the true meaning of its creed; We hold these truths to be self-evident: that all men are created equal",
"Morality cannot be legislated, but behavior can be regulated. Judicial decrees may not change the heart, but they can restrain the heartless.",
"Everybody can be great... because anybody can serve. You don't have to have a college degree to serve. You don't have to make your subject and verb agree to serve. you only need a heart full of grace. a soul generated by love.",
"He who passively accepts evil is as much involved in it as he who helps to perpetrate it.",
"Riots are the voices of the unheard.",
"I must confess that over the past few years I have been gravely disappointed with the white moderate. I have almost reached the regrettable conclusion that the Negro's great stumbling block in his stride toward freedom is not the White Citizen's Counciler or the Ku Klux Klanner, but the white moderate, who is more devoted to 'order' than to justice; who prefers a negative peace which is the absence of tension to a positive peace which is the presence of justice [...]",
];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a file consts.js and move this to the file as mlk_quotes.

Rename mlk_quote() to get_mlk_quote(). It's a standard (at least in python) to start functions with their action verb (since functions tend to perform actions).

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is part of the reason why a common foo function is do_something()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, if you have generic helper methods (not sure if you do), you can stick them in a utils.js file. very helpful

Comment on lines 8 to 9
const commands_json = fs.readFileSync("commands.json");
const added_commands = JSON.parse(commands_json).commands;
Copy link
Collaborator

Choose a reason for hiding this comment

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

const commands_file = fs.readFileSync("commands.json");
const commands = JSON.parse(commands_file).commands;

I'd rename these to be more explicitly descriptive

not sure if the second line causes a collision/is a reserved word, but if not, should be good

one standard is to avoid including typing in the variable name, but I'd argue that in this case commands_file is fine, since it does describe the object

and this is distinct from naming something like name_str or age_int

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since added_commands is only used in the internal logic of the add/edit commands methods, instead of storing this as a global var, I'd add a line to each of those functions to first action retrieve the current commands value

Also, not sure how js scoping works, but potentially, if you move all that logic to commands.js, you can just direct access commands, without having to do imports

you could then also update saveCommands to just take in the new commands that you want to save (not necessarily All Existing commands), and do:

  1. get all saved+existing commands
  2. loop through new commands and check for collisions
  3. if no collisions, just update all existing to include new commands
  4. write back to file

Copy link
Owner Author

Choose a reason for hiding this comment

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

added_commands is also used in message_main to check if the command was called by a user

Comment on lines 10 to 16
function saveCommands(commands) {
let save_commands_json = {
commands: commands,
};

fs.writeFileSync("commands.json", JSON.stringify(save_commands_json));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels convoluted to me

why not: ?

function saveCommands(commands) {
  fs.writeFileSync("commands.json", JSON.stringify({commands: commands}));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also stick this in commands.js

Comment on lines 18 to 19
//every 5 minutes, save commands to persist usage counts
setInterval(() => saveCommands(added_commands), 5 * 60 * 1000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

good comment, A+

Comment on lines 140 to 146
const command_word = user_parameters.shift();
const edit_command = added_commands.find((command) => command.command_word === command_word);
if (!edit_command) {
return `!${command_word} is not an existing command.`
}

const mod_only = user_parameters[user_parameters.length - 1] === `+m`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

same thought re destructuring

consider making a commands util to destructure and return command parts, that can be shared in these different functions

Copy link
Collaborator

Choose a reason for hiding this comment

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

also add a util for doesCommandExist, as this logic is repeated too

}
}

for (let added_command of added_commands) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a short comment describing this loop, cause it's not obvious at first glance

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably make this a commands.js function, maybe named getCommandInfo()

Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of looping through every persisted command and running the following logic, you can make your mods, count, and age checks first, and then retrieve the relevant command from the persisted set (if it exists)

this 1) makes you code run a modicum faster 2) removes a layer of nesting indents 3) i'd argue makes it a little more readable

@@ -75,35 +169,53 @@ const message_main = async (user_info, user_msg) => {
return respondToMatanbotMention(user_info);
}

const user_parameters = user_msg.split(" ");
user_command = user_parameters.shift().toLowerCase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

avoid recycling var names, it makes debugging harder

Comment on lines 205 to 206
} else {
added_command.usage_count = added_command.usage_count + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you only increment the command.usage_count if it's called by a mod? is that desired functionality?

Comment on lines 208 to 214
return added_command.response_array.reduce((cur_value, add_value) => {
if (add_value === `{count}`) {
return `${cur_value} ${added_command.usage_count}`
} else {
return `${cur_value} ${add_value}`
}
}, "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused about what this is doing, and too lazy to look it up

add a comment, or don't have a 6 line return. maybe make this a named variable prior to the return, and simply return that var

@matanp matanp merged commit 393c7f2 into main Feb 14, 2021
@matanp matanp deleted the mod_add_command branch February 14, 2021 00:12
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.

Mods Can Add Commands
2 participants