Skip to content

Conversation

@Atmois
Copy link
Contributor

@Atmois Atmois commented Dec 29, 2024

Description

Use a different searching system and also improved the code

Guidelines

  • My code follows the style guidelines of this project (formatted with Ruff)

  • I have performed a self-review of my own code

  • I have commented my code, particularly in hard-to-understand areas

  • I have made corresponding changes to the documentation if needed

  • My changes generate no new warnings

  • I have tested this change

  • Any dependent changes have been merged and published in downstream modules

  • I have added all appropriate labels to this PR

  • I have followed all of these guidelines.

How Has This Been Tested? (if applicable)

tested in atl.dev

Screenshots (if applicable)

n/a

Additional Information

n/a

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 29, 2024

Reviewer's Guide by Sourcery

This pull request refactors the wiki search functionality to use a more general approach and improves code readability. It introduces a new query_wiki function to handle searches for both ArchWiki and atl.wiki, replacing the separate query_arch_wiki and query_atl_wiki functions. Additionally, it adds a create_embed function to streamline the creation of embedded messages for search results.

Sequence diagram for the refactored wiki search process

sequenceDiagram
    participant User
    participant Bot
    participant WikiAPI

    User->>Bot: Send search query
    Bot->>Bot: query_wiki(base_url, search_term)
    Bot->>WikiAPI: GET request with query params
    WikiAPI-->>Bot: Return search results
    Bot->>Bot: create_embed(title, ctx)
    Bot-->>User: Send embedded response
Loading

Class diagram showing the Wiki cog structure after refactoring

classDiagram
    class Wiki {
        +bot: Tux
        +arch_wiki_api_url: str
        +atl_wiki_api_url: str
        +create_embed(title: tuple, ctx: Context) discord.Embed
        +query_wiki(base_url: str, search_term: str) tuple
        +wiki(ctx: Context)
        +arch_wiki(ctx: Context, query: str)
        +atl_wiki(ctx: Context, query: str)
    }
    note for Wiki "Refactored to use a single query_wiki method"
Loading

File-Level Changes

Change Details Files
Introduced a new function called query_wiki to handle wiki queries for both Arch and ATL wikis.
  • Created the query_wiki function to replace the existing separate query functions.
  • The new function takes the base URL as a parameter, making it more versatile.
  • Updated the command handlers to use the new query_wiki function.
  • The query_wiki function now uses the MediaWiki API's action=query and list=search parameters, which is a more standard way to perform searches and returns more information about the search results, including the URL of the page.
  • The query_wiki function now returns a tuple containing the title and URL of the first search result, or "error", "error" if no results are found.
  • The query_wiki function now logs the raw JSON response from the API using logger.info(data).
tux/cogs/utility/wiki.py
Created a helper function called create_embed to generate the Discord embed for search results.
  • Created the create_embed function to centralize the embed creation logic.
  • The function takes the search result title and the command context as input.
  • It handles the case where no search results are found by returning an error embed.
  • The command handlers now call the create_embed function to create the embed message.
  • The create_embed function now takes a tuple containing the title and description of the search result as input. If the first element of the tuple is "error", it indicates that no search results were found and an error embed is created. Otherwise, an info embed is created with the title and description of the search result.
tux/cogs/utility/wiki.py
Refactored the wiki search command handlers to use the new helper functions.
  • Updated the arch_wiki and atl_wiki command handlers to use the new query_wiki and create_embed functions.
  • Simplified the logic in the command handlers by delegating the query and embed creation to the helper functions.
  • Removed redundant code in the command handlers related to embed creation and error handling.
  • The command handlers now call the query_wiki function with the appropriate base URL for the wiki being searched.
  • The command handlers now call the create_embed function with the search result and the command context to create the embed message.
tux/cogs/utility/wiki.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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:

  • Could you explain the rationale for switching from the opensearch API to the query API? This is a significant change that should be documented in the PR description.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟡 Security: 1 issue found
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 84 to +86
data = response.json()
return (data[1][0], data[3][0]) if data[1] else ("error", "error")
logger.info(data)
if data.get("query") and data["query"].get("search"):
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨 suggestion (security): Consider logging only relevant fields to avoid potential sensitive data exposure

Suggested change
data = response.json()
return (data[1][0], data[3][0]) if data[1] else ("error", "error")
logger.info(data)
if data.get("query") and data["query"].get("search"):
data = response.json()
search_data = data.get("query", {}).get("search", [])
logger.info(f"Search returned {len(search_data)} results")
if search_data:

discord.Embed
The created embed message.
"""
if title[0] == "error":
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (code-quality): We've found these issues:

Comment on lines +87 to +88
search_results = data["query"]["search"]
if search_results:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)

Suggested change
search_results = data["query"]["search"]
if search_results:
if search_results := data["query"]["search"]:

@kzndotsh kzndotsh merged commit 64c3c59 into main Dec 29, 2024
9 checks passed
@kzndotsh kzndotsh deleted the wiki-capatalise branch December 29, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants