Wikiguess Game#618
Conversation
Kronifer
left a comment
Kronifer
left a comment
There was a problem hiding this comment.
Everything looks great good job!
Xithrius
added
area: backend
Xithrius
added
status: needs review
ChrisLovering
left a comment
ChrisLovering
left a comment
There was a problem hiding this comment.
Hi, I started to review the code, but found it harder than I should to follow it, mostly due to the lack of comments. Could you add some useful comments around this new cog that explains why you chose to do things in certain ways?
|
Also, could you update the reason section of your PR description? This section is supposed to be used to explain why you made the implementation decisions you did. This is not a place to describe the whole reason for the cog, or who approved it. |
RohanJnr
left a comment
RohanJnr
left a comment
There was a problem hiding this comment.
Excellent work 💯 ! Just a few changes and bug encounters.
kosayoda
left a comment
kosayoda
left a comment
There was a problem hiding this comment.
The current game loop is fragile.
DEFAULT_QUESTION_LIMITis hardcoded to 6 in 4430760, which seems to be because there are only 7 questions in theretrocategory.- Using the default question limit bypasses the topic length check
- The game loop only ever exits if the number of done questions go above the question limit
- The current question is retrieved in an infinite loop calling
random.choiceon the available questions and checking if it's done
These points result in the game (and the bot) hanging if the number of available questions for a topic is lower than the hardcoded DEFAULT_QUESTION_LIMIT. Since the wikipedia questions are auto-populated (again with an arbitrary hardcoded value of 10), take note of the above scenario when adding the ratelimit to the API.
Ideally, the game loop should be refactored to better retrieve a question for a topic, the default question limit should have a comment describing why it's set to 6, and the default question limit should not bypass the topic questions length check. I don't feel like the fixes for these should go in this PR, so just keep in mind the above quirks for this game so the bot still works for this topic until the fix is made. A separate issue should be created after this PR.
wookie184
left a comment
wookie184
left a comment
There was a problem hiding this comment.
Nice stuff, I'm approving this as it does seem to work, although there are two comments I would like to make.
Firstly, while the randomness of the articles chosen is nice, there are a lot of wikipedia articles, to the extent that usually picking a random wikipedia page will give you some obscure topic you have no idea about. For example:
[redacted] is a village in Nowkand Kola Rural District, in the Central District of Qaem Shahr County, Mazandaran Province, Iran. At the 2006 census, its population was 24, in 10 families.
and as well as this some also seem like they could have multiple answers, e.g.
[redacted] is a village in Kozhikode district in the state of Kerala, India.
In my testing I couldn't find any questions which I could answer and this makes the game not particularly interesting. A solution to this would be to instead store a list of manually chosen wikipedia pages that can be chosen from.
I also think the current hourly refreshing of questions isn't great. Most days the .quiz command will not be used at all, meaning the bot is making 24 unnecessary API requests. As well as this, often when it is used it will be used multiple times in the same hour, meaning you will get repeated questions.
I think it would have been ok just to make a request per command ran, or if the api ratelimits aren't high enough to do that it should only refresh the list of questions if the current ones have been used.
Also there is currently a race condition where if you run the .quiz wikipedia on startup before get_wiki_questions has finished you'll get a KeyError as the questions aren't there yet.
It could be worth opening an issue for these changes once this PR is merged if you don't want to tackle them now.
wookie184
added
status: waiting for author
janine9vn
left a comment
janine9vn
left a comment
There was a problem hiding this comment.
This PR will conflict with our current code base since we merged in the Type Annotations PR.
You should pull down the recent changes and resolve those conflicts.
Otherwise from my testing the game plays just fine.
This commit also moves all the 'dynamic' question generator to a separate class. Closes: #446
If the task hit max error fetches, which is 3 currently, it would remove wikipedia from listed categories and not add it to loaded questions. If it doesn't hit max fetches, then it adds them.
* add 30 questions each under the categories "cs" and "python" add the two categories into the code and modify the starting phase Co-authored-by: Xithrius <15021300+Xithrius@users.noreply.github.com> Co-authored-by: ToxicKidz <78174417+ToxicKidz@users.noreply.github.com>
Since random wikipedia article guess questions weren't really "knowledgeable", no one could really guess it or gain any "good" knowledge from them, so after asking wookie (this commits mentions his review comments above also), I decided to use these.
Co-authored-by: wookie184 <wookie1840@gmail.com>
If the number of questions are less then the default limit which can happen in the case of wikipedia guess game as it is dependent on the most read articiles on wikipedia, it would create a infinite loop sending us into infinite amount of errors, so let's prevent that, thanks wookie
We can use r"", a raw string, here to make it clear that \* and \s aren't supposed to be handled as escape sequences and just use the "raw string".
Originally, before this commit, we checked the number of questions left by comparing `len(done_questions) > self.question_limit`, so question limit had to be 1 since if it wasn't we would compare 7 > 7, which would be false and then it would send another question. To correct this bug, we now use == comparision on the two, so if the number of done questions is same as the question limit it means that the round is over. I have changed the relevant parts of the code to reflect this change i.e. where-ever we did +-1 due to the off by one bug.
Since the title can sometimes contain punctuations making it very difficult to get the matching answer to the question, we originally removed all such questions. This took the question count down :( and wasn't an effective way. Therefore now we keep them but as normalized, yay! I have also updated the code documentation to make the process much clearer to anyone reading the "normalizing" code section of the wiki questions generator.
Wasn't fitting in character limit so shorterned it ^^ lol. Okay, getting to the point, this mentions fix error's comment of making quiz entry except the answers as a list and not as a string which could a comma joined list. The same structure was in the json resource, where multiple answers where joined with commas. This didn't allow you to use commas in answers. So I went ahead and did a bit more than requested to change the json structure and make `answers` a list. Also now all questions are in the form of the quiz entry to keep it same through out the code and var tolerance has become a valid param in QuizEntry, this is done because it was differing between questions, if not needed this would make the process to add `var_tol` as a argument to the json easier. And that's it!
Xithrius
added
category: fun
Xithrius
removed
the
status: needs review
11 participants
Relevant Issues
Closes #446
Description & Reasoning
Tis PR adds the wikiguess game. To implement this I have used the random wikipedia article API, to get the title, extract. The questions are formed using the extract, which is then run through a replace function, to remove the title if it is present in the extract with
[redacted].Another check I have added while fetching the wikiguess question, is to check whether the title contains any non alpha-numeric character (
-+-*/[]{}()!@#$%^&). As it becomes difficult to answer when the answer is like "Felsőkelecsény" or " Camp Lake, Wisconsin" and you answer "Camp Lake".Screenshots
Did you:
pipenv run lint)?