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

Fix check pyproject.toml file existent #1408

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KennyDizi
Copy link
Contributor

@KennyDizi KennyDizi commented Dec 22, 2024

PR Type

Bug fix


Description

  • Fixed a bug in version detection logic where the warning about missing pyproject.toml was incorrectly indented
  • The warning message now properly triggers when the file is missing, rather than when Python version is < 3.11
  • Maintains existing fallback to pip package version when needed

Changes walkthrough 📝

Relevant files
Bug fix
utils.py
Fix version check logic for pyproject.toml file                   

pr_agent/algo/utils.py

  • Fixed incorrect indentation in version check logic
  • Moved the 'else' clause outside the Python version check block
  • Improved error handling for missing pyproject.toml file
  • +2/-2     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🏅 Score: 95
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The function should handle potential IOError/OSError when opening pyproject.toml file, as file permissions or other issues could prevent reading even if the file exists

                with open("pyproject.toml", "rb") as f:
                    data = tomllib.load(f)

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect error handling logic that fails to properly handle Python version requirements and file existence cases

    The else clause for handling missing pyproject.toml is incorrectly indented and only
    executes for Python versions < 3.11. Move it to align with the outer if statement to
    properly handle all cases where the file doesn't exist.

    pr_agent/algo/utils.py [1119-1129]

     if os.path.exists("pyproject.toml"):
         if sys.version_info >= (3, 11):
             import tomllib
             with open("pyproject.toml", "rb") as f:
                 data = tomllib.load(f)
                 if "project" in data and "version" in data["project"]:
                     return data["project"]["version"]
                 else:
                     get_logger().warning("Version not found in pyproject.toml")
    +    else:
    +        get_logger().warning("Python 3.11+ required to parse pyproject.toml")
     else:
    -    get_logger().warning("Unable to determine local version from pyproject.toml")
    +    get_logger().warning("pyproject.toml file not found")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies a logical error in the error handling structure and proposes a more robust solution that properly handles both Python version requirements and file existence cases. The improved code provides clearer error messages and better logical flow.

    8
    Add proper error handling for file operations to prevent crashes on file access or parsing errors

    Add error handling for file I/O operations when reading pyproject.toml to catch
    potential permission or corruption issues.

    pr_agent/algo/utils.py [1122-1123]

    -with open("pyproject.toml", "rb") as f:
    -    data = tomllib.load(f)
    +try:
    +    with open("pyproject.toml", "rb") as f:
    +        data = tomllib.load(f)
    +except (IOError, tomllib.TOMLDecodeError) as e:
    +    get_logger().warning(f"Error reading pyproject.toml: {e}")
    +    return version('pr-agent')
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important error handling for file I/O operations that could prevent application crashes due to permission issues or corrupted TOML files. This is a valuable defensive programming practice.

    7
    • Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.

    Comment on lines +1128 to +1129
    else:
    get_logger().warning("Unable to determine local version from pyproject.toml")
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    i am not sure i agree with this change.

    If I have a pip package, than I won't find a "pyproject.toml" file. but its ok. it does not justify a "warning" message.

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    That's a good point, this is the old code. You can see this code is not reachable
    CleanShot 2024-12-23 at 18 58 26
    And it doesn't have the same code block with if os.path.exists("pyproject.toml"):

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    This is the new code
    CleanShot 2024-12-23 at 19 08 32

    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    image

    maybe its due to your python version ?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    I'm not sure, I'm using 3.13. Let me check it with 3.12 and get back to you afterward.

    Copy link
    Collaborator

    @mrT23 mrT23 Dec 24, 2024

    Choose a reason for hiding this comment

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

    i am saying i dont see unreachable code warning on my platform.

    And I am not sure from the code why it should be, always, unreachable.
    Maybe its an issue of your specific enviroment

    @chungocchien
    Copy link

    Preparing review...

    1 similar comment
    @chungocchien
    Copy link

    Preparing review...

    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