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

[feature] FAQ bot add, list, and define #71

Closed
wants to merge 6 commits into from
Closed

[feature] FAQ bot add, list, and define #71

wants to merge 6 commits into from

Conversation

jackwherry
Copy link
Collaborator

@jackwherry jackwherry commented Dec 13, 2020

Purpose of pull request

Related Issue: resolves #30

Pull request checklist

  • Branch and PR are named correctly
  • Updated relevant documentation
  • Tested with a tester bot
  • Wrote unit tests for changes
    3/3 done

PR-specific checklist

  • Responds to ++faq add
  • Responds to ++faq list
  • Responds to ++faq define
  • Responds to ++faq delete
  • Responds to ++faq list -i
  • Handles duplicates with ++faq define
  • Restricts commands that write to the database to certain roles

Testing instructions

++faq

@brendacs brendacs changed the title Draft: [feature] FAQ bot [WIP][feature] FAQ bot Dec 13, 2020
@nikgil
Copy link
Member

nikgil commented Dec 15, 2020

Do you want to partition out the commands a bit to different PRs? Would be easier to review that way and incrementally upload to main bot without having to do massive rollbacks (if god forbid we had to)

@jackwherry jackwherry changed the title [WIP][feature] FAQ bot [WIP][feature] FAQ bot add, list, and define Dec 24, 2020
@jackwherry jackwherry marked this pull request as ready for review December 25, 2020 00:57
@jackwherry jackwherry changed the title [WIP][feature] FAQ bot add, list, and define [feature] FAQ bot add, list, and define Dec 25, 2020
let details = dedent(`**${faq.term}**: ${faq.definition}`);

if (addReferences) {
details += dedent(`
Copy link
Member

Choose a reason for hiding this comment

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

maybe prettier do do \n instead of this

Copy link
Member

Choose a reason for hiding this comment

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

agree


if (addId) {
details += dedent(`
ID: ${faq._id}`);
Copy link
Member

Choose a reason for hiding this comment

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

same as above comment

import addFaq from './subCommands/addFaq';
import client from '../../client';
import { commandHandler } from '../../utils';
// import deleteFaq from './subCommands/deleteFaq';
Copy link
Member

Choose a reason for hiding this comment

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

please don't leave comments in the code.

import { commandHandler } from '../../utils';
// import deleteFaq from './subCommands/deleteFaq';
import defineFaq from './subCommands/defineFaq';
// import editFaq from './subCommands/editFaq';
Copy link
Member

Choose a reason for hiding this comment

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

ditto


let newFaq = await Faq({
definition: args.slice(1).join(' '),
term: args[0]
Copy link
Member

Choose a reason for hiding this comment

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

@brendacs think that we would need to create multi word terms? Some FAQs in my mind may definitely be more than one word (examples: functional programming, data structures)

Copy link
Member

Choose a reason for hiding this comment

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

i agree, that was my thought from the beginning

Copy link

@payne911 payne911 Jan 30, 2022

Choose a reason for hiding this comment

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

@nikgil what about simply using hyphens instead of spaces? Spaces are generally understood as carrying semantic within commands, so I think it's safer (for extensibility) to keep it to a "single-word".

E.g. function-programming and data-structures

let allFaqs = '**All FAQs:**\n\n';

faqs.forEach(
(faq) => (allFaqs += `${getFormattedFaq(faq, true, showIds)}\n\n`)
Copy link
Member

Choose a reason for hiding this comment

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

later problem is getting this paginated

@github-actions
Copy link

This issue is stale because it has been open at least 120 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@payne911
Copy link

payne911 commented Jan 30, 2022

Any reason why ++faq delete was scrapped?

Also, as a personal note, I think it'd be interesting to have aliases. For example: ++faq d <word> as a shortcut to ++faq define <word>.

++fd <word>, while being nice and short, might however not be a good idea because it should probably be seen as taking up the place of another command. It makes a lot more sense to have diminutives added as children of the main command's tree.

@nikgil
Copy link
Member

nikgil commented Feb 7, 2022

Not scrapped per se as much as can be done in multiple PRs since they are pretty independent of each other

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

Successfully merging this pull request may close these issues.

[feature] FAQ bot should respond to '++faq add'
4 participants