Skip to content

Conversation

Objectivitix
Copy link
Contributor

@Objectivitix Objectivitix commented May 7, 2021

Relevant Issues

Closes #719

Description & Implementation

TL;DR - added multiple possible answers, images, dynamically generated questions and two new categories (60 questions in total)

PART ONE:
After gathering a database of questions and answers, I incorporated them into the json file. Some questions requiring images are supplemented with an "img_url" entry, and some requiring dynamic generation (e.g. Math Q1, a random system of linear equations with two unknowns) have an "dynamic_id" entry, which will come into use later.

PART TWO:

  1. Added two new categories to the quiz: "math" and "science"! Also fixed some original flaws in the code so the abandoned category "retro" can function properly. Updated the documentation string in quiz_game to include these categories.
  2. Implemented a "multiple possible answers" feature, where if a question can have multiple correct answers (usually synonyms and significant variations of a word which won't be tolerated by fuzz) they will all be correct. These correct answers will also be listed in the answer embed (send_answer), and are separated by commas in the json file.
  3. Added support for questions requiring images (with an "img_url" entry) in the quiz_game method.
  4. Added some preparation for the next commit, which will add in the formatting functions dictionary. More on that later.
  5. Added a feature where the amount of questions can be specified with .quiz <category> <questions>. Also changed the default from 5 questions to 8 questions. Added error embeds which will be sent when questions is larger than the amount of questions in that category, or if it's less than 0 (0 just makes it default).
  6. Lightly refactored the code, making it easier on the eyes by adding some logical spacing in dense areas, using literal concatenation on some long strings, etc.

PART THREE:
This is where all the dynamic questions come to life! Each dynamic question has an unique "dynamic_id", which corresponds to a function taking its "question" and "answer" as input, formats them, and returns them in a tuple. This is how the questions will be dynamically generated.

Images

Normal Q&A
Dynamically generated question
Question with image
Error embeds

Did you:

@Objectivitix Objectivitix marked this pull request as draft May 7, 2021 17:19
@Objectivitix
Copy link
Contributor Author

converting to draft because i have to fix some linting fails

@Objectivitix Objectivitix marked this pull request as ready for review May 7, 2021 17:33
@Objectivitix
Copy link
Contributor Author

all lint failures fixed, ready for review!

@Objectivitix
Copy link
Contributor Author

Objectivitix commented May 7, 2021

REGARDING THE CS AND PYTHON QUESTIONS:

We... just didn't get that much questions, so I didn't add them into the database. Our list can be found here, and once we get 30 questions on each I might open a new PR

Copy link
Contributor

@Kronifer Kronifer left a comment

Choose a reason for hiding this comment

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

Lgtm, tested and works good

@Objectivitix Objectivitix changed the title Add "math" and "science" categories to the .quiz command. Add "math" and "science" categories to the .quiz command (and more in desc) May 8, 2021
@Objectivitix Objectivitix requested a review from anand2312 May 9, 2021 18:09
@Xithrius Xithrius added area: backend Related to internal functionality and utilities season: evergreen status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features type: feature Relating to the functionality of the application. labels May 12, 2021
@Objectivitix
Copy link
Contributor Author

backend? i'm adding more stuff for user features
hmm... might be because the code had been heavily modified to fit with the new image questions and dynamic questions lol

Copy link
Contributor

@Shivansh-007 Shivansh-007 left a comment

Choose a reason for hiding this comment

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

As a pure code review, this looks good to me :shipit:

@Objectivitix
Copy link
Contributor Author

woohoo

@Akarys42 Akarys42 dismissed their stale review May 13, 2021 13:19

Requested changes applied

self.bot = bot
self.questions = self.load_questions()

self.game_status = {} # A variable to store the game status: either running or not running.
Copy link
Member

Choose a reason for hiding this comment

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

Well, it's not very good that we have a piece of code nobody can understand. Code should be understandable by those who read it. If it's not, you should investigate and fix it.

From what I was able to gather from the code, game_status is a dict that maps a channel ID to a boolean flag. Its purpose is to make sure that at most one quiz is present in a channel, but to allow multiple quizzes in multiple channels.

So this is what I would do:

self.is_game_running: Dict[int, bool] = {}  # maps channel ID to whether a quiz is running in that channel

Now, you also have two other variables:

        self.game_owners = {} # A variable to store the person's ID who started the quiz game in a channel.
        self.player_scores = {}  # A variable to store all player's scores for a bot session.
        self.game_player_scores = {}  # A variable to store temporary game player's scores.

Do you see a pattern? All these 4 dicts take a channel ID and return a game state. So what you could do is put all that into one object:

class Game:
    def __init__(self, owner_id: int):
        self.owner_id = owner_id
        self._scores: Dict[int, int] = {}

    def add_score(self, player_id: int, score_delta: int) -> None:
        self._scores[player_id] = self._scores.get(player_id, 0)  + score_delta

    def player_scores(self) -> Sequence[Tuple[int, int]]:
        """Returns a list of (user_id, score) tuples"""
        return list(self._scores.items())

inside the Cog class:

self.games: Dict[int, Game] = {}  # maps channel ID to its current game

self.player_scores is actually not used at all (double-check, but it seems so), so remove it.

except for the major refactoring, which would probably be in a separate pr
smh i have to stop making these errors lmao, definitely going to see that precommit thing after this
@Objectivitix
Copy link
Contributor Author

@decorator-factory your changes have all been implemented (except for major refactoring) :D

Copy link
Member

@decorator-factory decorator-factory left a comment

Choose a reason for hiding this comment

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

LGTM

@Xithrius
Copy link
Member

@Objectivitix please resolve these conflicts.

@Objectivitix
Copy link
Contributor Author

@Xithrius All conflicts have been resolved. Two are caused by the spring cleanup I believe, and so I selected the spring cleanup ones

@Xithrius
Copy link
Member

Thank you, I'll do a final test and then give my thoughts.

Code looks good to me as of now.

Copy link
Member

@Xithrius Xithrius left a comment

Choose a reason for hiding this comment

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

It works well.

@Xithrius Xithrius merged commit a5d138e into python-discord:main May 16, 2021
@Objectivitix
Copy link
Contributor Author

ZETAPOGGERS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to internal functionality and utilities type: enhancement Changes or improvements to existing features type: feature Relating to the functionality of the application.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Math" and "Science" category in .quiz