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

Pure Pulsars #7

Open
wants to merge 390 commits into
base: main
Choose a base branch
from
Open

Pure Pulsars #7

wants to merge 390 commits into from

Conversation

janine9vn
Copy link
Contributor

No description provided.

Koisu-unavailable and others added 30 commits July 21, 2024 14:47
This just send a message with the answer when the give up button is pressed, it doesn't actually perform any necessary cleanup.
This modifies the original message to not have the buttons when the user gives up the game.
Allows using the `logging` module to pass log messages to discord's default logger. ALSO adds using the environment variable `LOG_LEVEL` to set the logging level, with the current default being INFO. So you can run `LOG_LEVEL=DEBUG python src/main.py` to get a DEBUG-level logging experience.
DEBUG is very messy, and this info is slightly more useful to maintenance than the common DEBUG output
(wiki-guesser) "Give Up" button, logging update
Added commands.bot.tree instead of Client and added a sync command under the prefix of /
Added it for more readable timestamps for the User info command
added it to the user info for an easier piece of info.
made things work. added the on_join event for syncing on join, and also updated the /sync command so we can run it.
-----
Another command, ``/reset-scores``, can be used to reset the scores of
all users in a server.
:w
Copy link

Choose a reason for hiding this comment

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

What does :w represent here?


Notes:
-----
Another command, ``/reset-scores``, can be used to reset the scores of
Copy link

Choose a reason for hiding this comment

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

Is it supposed to have three backticks instead?


await interaction.followup.send(embed=embed)
except CommandInvokeError as e:
logging.info("Leaderboard:\nException: %s", e)
Copy link

@Jayy001 Jayy001 Aug 21, 2024

Choose a reason for hiding this comment

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

I think this should be logging.error instead, also could utilise an f-string

await interaction.response.defer(thinking=True)
board = await DATA.get_server(ser_id)
lead = list(board.values())
lead.sort(key=_sort_leaders, reverse=True)
Copy link

Choose a reason for hiding this comment

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

I think a lambda would be cleaner, lead.sort(key=lambda user: user.score()) as the function is only being used here.

board = await DATA.get_server(ser_id)
lead = list(board.values())
lead.sort(key=_sort_leaders, reverse=True)
lead = lead[0:10]
Copy link

Choose a reason for hiding this comment

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

Could we get a comment to explain where 10 comes from?


COPY . .

RUN "ls"
Copy link

Choose a reason for hiding this comment

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

Is ls needed here?


WORKDIR /app

COPY requirements.txt requirements.txt
Copy link

Choose a reason for hiding this comment

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

Can we move the COPY . . part here and remove this line?

Copy link

Choose a reason for hiding this comment

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

Ah, this seems to be the correct approach due to how the layers work with building a container.

Copy link

Choose a reason for hiding this comment

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

What are these cache files needed for?

Copy link

Choose a reason for hiding this comment

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

I like these meeting notes

Copy link

Choose a reason for hiding this comment

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

I'm not going to review this because its not been implemented

discord.Embed: The embed message.

"""
embed_msg = Embed(title=summary["Title"], description=summary["Intro"], url=summary["URL"], color=0xFFFFFF)
Copy link

Choose a reason for hiding this comment

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

Could we add a comment of what the colour is?

try:
# Generate a summary and return an embed message
text = article.extract(intro=False)
response = model.generate_content(text)
Copy link

@Jayy001 Jayy001 Aug 22, 2024

Choose a reason for hiding this comment

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

This is an interesting way of getting the response from the AI, good catch on the try/except. However, I think there should be a note/warning to the user about the potential miss-information from using a LLM.

value=user_data["score"],
inline=False,
)
try:
Copy link

Choose a reason for hiding this comment

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

Could we pre-generate the value variable first, instead of using another nested try/except?

value = (user_data["wins"] / user_data["failure"]) if user_data["failure"] > 0 else user_data["wins"]

"""Determine animal's weight ranges based on article text."""
api_url = f"https://api.api-ninjas.com/v1/animals?name={animal_name}"
async with (
aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=20)) as session,
Copy link

Choose a reason for hiding this comment

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

This is quite a long timeout, won't the discord interaction timeout by then? Is it worth reducing it?

try:
api_url = "https://en.wikipedia.org/wiki/Special:RandomInCategory?wpcategory=Mammals of the United States"
async with (
aiohttp.ClientSession(headers={"UserAgent": UA}, timeout=aiohttp.ClientTimeout(total=20)) as session,
Copy link

Choose a reason for hiding this comment

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

Same long timeout here

if weight_phrases:
weight_ranges = [UREG.Quantity(weight) for weight in weight_phrases]
if len(weight_ranges) % 2 == 0:
return [weight_ranges[0], weight_ranges[1]]
Copy link

Choose a reason for hiding this comment

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

I don't understand the point of this code, why are we checking if its even/odd? Could we adda comment

loc = str(response.real_url)
title = loc.split("=")[1].split("&")[0]
article = await search_wikipedia(title)
animal_name = article.title().split(" ")[-1]
Copy link

Choose a reason for hiding this comment

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

This can error if search_wikipedia doesn't return anything.

from button_class import ExcerptButton, GiveUpButton, GuessButton, LinkListButton, _Button, _Ranked
from wikiutils import make_embed, rand_wiki, win_update

ACCURACY_THRESHOLD = 0.8
Copy link

Choose a reason for hiding this comment

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

What do these constants represent?

await interaction.followup.send(content=msg, embed=embed)
if ranked:
user = interaction.user
for i in [interaction.guild_id, 0]:
Copy link

Choose a reason for hiding this comment

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

Why do we go in 0 element as well?


excerpt = article.extract(chars=1200)

for i in article.title().split():
Copy link

Choose a reason for hiding this comment

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

Will this censor the word if its in FULL UPPERCASE?

excerpt = excerpt.replace(i, "~~CENSORED~~")
excerpt = excerpt.replace(i.lower(), "~~CENSORED~~")

sentances = [i for i in excerpt.strip("\n").split(".") if i]
Copy link

@Jayy001 Jayy001 Aug 22, 2024

Choose a reason for hiding this comment

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

*sentences, I think we could have a better variable name than i - and a comment?

self._default_app = firebase_admin.initialize_app(
cred_obj,
{
"databaseURL": "https://pure-pulsars-default-rtdb.firebaseio.com/",
Copy link

Choose a reason for hiding this comment

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

Need's to be taken from user input!

NullUserError: If the user doesn't exist.

"""
"""self._ref = db.reference("/users")"""
Copy link

Choose a reason for hiding this comment

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

What's this for?



# activates the commands
sync.main(client.tree)
Copy link

Choose a reason for hiding this comment

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

Is there a better way to do this then all these lines?

"""
result = Page(site, title=query)

if not result.exists() or result.isRedirectPage():
Copy link

Choose a reason for hiding this comment

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

Does the redirect page take you to to article?

return get_best_result(results, query)

try:
return result
Copy link

Choose a reason for hiding this comment

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

How would this error?

await DATA.add_user(uid, new_user, guild)


async def win_update(guild: int, user: User, score: int) -> None:
Copy link

Choose a reason for hiding this comment

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

Would it be better to have a single update function which you can the pass in in the win/loss argument instead of having two functions?

articles = [article for article in articles if self.article_has_categories(article)]

try:
return articles
Copy link

Choose a reason for hiding this comment

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

How would this error?

Comment on lines +466 to +472
articles = {a for articles in title_articles.values() for a in articles}

if any(article is None for article in title_articles.values()):
titles_not_found = [title for title, article in title_articles.items() if article is None]
message = f"No articles found for the following titles: {titles_not_found}"

raise ArticleGeneratorError(message)
Copy link

Choose a reason for hiding this comment

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

Could we get some comments here?

@Jayy001
Copy link

Jayy001 commented Aug 22, 2024

Hey everyone, fantastic project! The logo and website added a great personalised touch, and setting up the bot was a breeze. I've done a review of the codebase, and had a play around with the features - here are my thoughts on what I liked and what could be improved on.

Project overview

  • Well structured project, with the modules in their appropriate place. Using different folders to keep the discord interface and database access separate makes the codebase easy to use.
  • Each command is its own module, which makes organisation much easier, and each function can be worked on separately.
  • Consistent use of docstrings — simple to get a high level overview of what each function does
  • README.md documentation is very detailed, made running the bot a breeze. The meeting notes, are also quite insightful. Although there are quite a lot of external services to sign up to...
  • Commands work as expected and give a clear response, bar a few small bugs
  • Abided to the “Information overload” brief well.

Commit overview

  • Started strong with most commit messages clearly showing what had been worked on
  • Few inconsistencies towards the end with a few default messages (e.g. update file) without explaining what it added
  • Small changes instead of large PRs. 👍🏻

Codebase overview

  • Docstrings are included everywhere, some functions could do with a bit more explanation to the docstring
  • Typehints are inconsistent — some files have them while others don't. I can understand for variables which purpose is obvious, but in quite a few cases, it would benefit from having them.
  • Well structured and modular, utils module is especially great.
  • Needs some comments on code that operation isn't entirely obvious (or with magic numbers). Same with constants at the top!
  • Some variable names aren't named well. See my review.
  • I think there's too much of logging.info being used instead of logging.error, especially for when the program catches an error.

Usage overview

  • Help command could do with being more detailed, including the parameters needed.
  • I had no troubles with running guessr, random , search, reset-score, never, rabbit-hole and user-info.
  • Timeout for commands was OK, could be a bit longer, but not sure if thats a discord limitation.
  • NSFW Content! There are no restrictions on the articles given, so I did have a couple of instances where NSFW articles were being used (from the random or guess command). Something to check against this would be a great improvement!
  • wiki-animal: Sometimes errored for no apparent reason. When entering the weight, the threshold for accepted answer seems to be pretty high (i.e I guessed 1kg for a moose which was accepted)
discord.app_commands.errors.CommandInvokeError: Command 'wiki-animal' raised an exception: AttributeError: 'NoneType' object has no attribute 'title'
  • leaderboard: Command errors if no games has been played yet
discord.app_commands.errors.CommandInvokeError: Command 'leaderboard' raised an exception: AttributeError: 'NoneType' object has no attribute 'values
  • rabbit-hole: I like the idea of this command using a LLM to sum it up. However, I think a warning should be given as they are well known to hallucinate information which could potentially miss-inform the user.
  • random: I think an idea of not using the popular articles would be good as an optional parameter.

Other

  • Thank you for using Docker!! Makes running it easy.
  • I'm not quite sure what the cache files are used for
  • Neither what the never command does
  • I had to change the firebase URL in the python file, something that wasn't mentioned in the README

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.

8 participants