-
-
Notifications
You must be signed in to change notification settings - Fork 41
Add fun fact command #446
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
Add fun fact command #446
Conversation
Reviewer's Guide by SourceryThis pull request adds a new 'fun fact' command to the bot, which provides users with random Linux-related trivia. The implementation includes a new Python file with a Fact class that inherits from commands.Cog, containing a list of Linux facts and a command to display a random fact using Discord embeds. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Atmois - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
tux/cogs/fun/fact.py
Outdated
| "Linux's 1.0 release was in March 1994.", | ||
| "Less than 1% of the latest kernel release includes code written by Linus Torvalds.", | ||
| "Linux is used by every major space programme in the world.", | ||
| "Approximatly 13.3% of the latest Linux kernel is made up of blank lines.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (typo): Correct the spelling of 'Approximately' in the fact.
Ensuring accuracy in the facts presented enhances the credibility and professionalism of the bot.
tux/cogs/fun/fact.py
Outdated
| "95% of the Linux kernel is written in the C programming Language. Assembly language is the second most used language for Linux at 2.8%.", | ||
| "The first kernel version - Version 0.01 - contained about 10,000 lines of code.", | ||
| "96.3% of the top 1,000,000 web servers were reported to run on Linux.", | ||
| "In the early 2000's Steve Jobs - The then CEO of Apple - offered a job to Linux Torvalds which Torvalds declined.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (typo): Correct 'Linux Torvalds' to 'Linus Torvalds' and consider rephrasing for clarity.
This fact contains a typo in Linus Torvalds' name and could be rephrased for better readability. Consider: 'In the early 2000s, Steve Jobs, then CEO of Apple, offered a job to Linus Torvalds, which Torvalds declined.'
| "In the early 2000's Steve Jobs - The then CEO of Apple - offered a job to Linux Torvalds which Torvalds declined.", | |
| "In the early 2000s, Steve Jobs, then CEO of Apple, offered a job to Linus Torvalds, which Torvalds declined.", |
|
You can add this too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Atmois - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider completing the feature before submitting the PR, or provide more details about the remaining work in the PR description.
- To improve scalability and maintainability, consider moving the list of facts to a separate file or database.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
@Atmois is it fine if i can unmark as draft or should i wait until you add more facts |
|
Go ahead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Atmois - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider completing the feature before submitting a pull request, as indicated by the 'Unfinished' note in the description.
- For better maintainability, consider moving the list of facts to a separate configuration file or database in the future, especially if you plan to add more facts.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| "96.3% of the top 1,000,000 web servers were reported to run on Linux.", | ||
| "In the early 2000s, Steve Jobs, who at the time was the CEO of Apple, offered a job to Linus Torvalds to work on OSX which Torvalds declined.", | ||
| "Linus Torvalds said that he would have never created Linux if FreeBSD had been available at the time.", | ||
| "Linux is used by every major space programme in the world including NASA, SpaceX, and the European Space Agency.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Remove redundant fact
This fact is a duplicate of the one on line 8. Consider removing one of these instances to avoid repetition.
| "Linux is used by every major space programme in the world including NASA, SpaceX, and the European Space Agency.", | |
| # Removed duplicate fact about space programs using Linux |
|
|
||
|
|
||
| class Fact(commands.Cog): | ||
| def __init__(self, bot: commands.Bot) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Optimize fact selection for better distribution
Consider shuffling the facts list in the __init__ method and selecting facts from the first 75% of the list in the fact method. This approach would provide a good balance between randomness and ensuring all facts are seen before repeats occur.
def __init__(self, bot: commands.Bot) -> None:
self.bot = bot
self.facts = []
self.load_and_shuffle_facts()
def load_and_shuffle_facts(self) -> None:
self.facts = [...] # Load facts here
random.shuffle(self.facts)
self.fact_index = 0
| name="fact", | ||
| aliases=["funfact"], | ||
| ) | ||
| async def fact(self, ctx: commands.Context[commands.Bot]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Improve or remove method comment
The current comment doesn't provide much additional information. Consider either removing it or expanding it to include more meaningful details about the method's behavior or purpose.
| async def fact(self, ctx: commands.Context[commands.Bot]) -> None: | |
| async def fact(self, ctx: commands.Context[commands.Bot]) -> None: | |
| """ | |
| Fetch and send a random fun fact to the user. | |
| This command retrieves a fact from an external API or database | |
| and posts it in the channel where the command was invoked. | |
| """ |
| class Fact(commands.Cog): | ||
| def __init__(self, bot: commands.Bot) -> None: | ||
| self.bot = bot | ||
| self.facts = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Consider scalability for fact storage
If there are plans to significantly expand the fact list in the future, you might want to consider a more scalable solution for storing and retrieving facts, such as loading from a file or database.
self.facts = self.load_facts_from_file('facts.json')
def load_facts_from_file(self, filename):
with open(filename, 'r') as file:
return json.load(file)
Unfinished as going to add more fun facts.
Summary by Sourcery
Add a new command to the bot that delivers random fun facts about Linux, enhancing user engagement with interesting trivia.
New Features: