Skip to content

Conversation

@sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Jul 26, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

  • In this PR I have moved all the information about the non-default file information that gets shipped as part of selenium package from MANIFEST.in to pyproject.toml file.

Motivation and Context

  • This eliminates the need of having a separate config file.
  • Also, recursive discovery of all packages can be specified in pyproject.toml file through table [tool.setuptools.packages.find] , the package information in setup.py can be eliminated and the same can be maintained in pyproject.toml.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Configuration changes


Description

  • Removed MANIFEST.in file and moved its contents to pyproject.toml.
  • Updated setup.py to eliminate package configuration, now handled by pyproject.toml.
  • Modified Bazel build configuration to remove reference to MANIFEST.in.

Changes walkthrough 📝

Relevant files
Configuration changes
setup.py
Remove package configuration from setup.py                             

py/setup.py

  • Removed package_dir and packages definitions.
  • Eliminated include_package_data setting.
  • +0/-21   
    BUILD.bazel
    Update Bazel build configuration                                                 

    py/BUILD.bazel

    • Removed reference to MANIFEST.in file.
    +0/-1     
    MANIFEST.in
    Remove MANIFEST.in file                                                                   

    py/MANIFEST.in

    • Deleted the MANIFEST.in file entirely.
    +0/-24   
    pyproject.toml
    Add package and data configuration to pyproject.toml         

    py/pyproject.toml

  • Added package discovery and data inclusion settings.
  • Included non-default file information.
  • +19/-0   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @sandeepsuryaprasad sandeepsuryaprasad changed the title [py] Getting rid of MANIFEST.in file by moving non-default file info to pyproject/toml [py] Getting rid of MANIFEST.in file by moving non-default file info to pyproject.toml Jul 26, 2024
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Configuration Error
    The selenium_package key in [tool.setuptools.package-data] might not be recognized by setuptools. Usually, the key should match the package name exactly, which is typically 'selenium' rather than 'selenium_package'.

    Missing Package Definitions
    The removal of 'package_dir' and 'packages' from setup.py without ensuring they are correctly handled in pyproject.toml could lead to issues in package discovery and installation.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 26, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the file pattern to ensure it matches the intended files

    *Ensure that the selenium.egg-info pattern correctly matches intended files. It
    appears to have a space which might cause it to not work as expected.

    py/pyproject.toml [17]

    -"selenium.egg-info *",
    +"selenium.egg-info/*",
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Correcting the file pattern is crucial to ensure that the intended files are matched correctly, preventing potential build issues.

    9
    Maintainability
    Use more specific file patterns to avoid including unwanted files

    It's recommended to use a more specific glob pattern or explicitly list the files in
    the selenium_package to avoid unintentionally including unwanted files.

    py/pyproject.toml [11-21]

     selenium_package = [
                         "*.py", 
                         "*.rst",
                         "*.json", 
                         "*.xpi", 
                         "*.js",
    -                    "selenium.egg-info *",
    +                    "selenium.egg-info/*",
                         "selenium-manager", 
                         "selenium-manager.exe",
                         "CHANGES",
                         "LICENSE"
                         ]
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using more specific glob patterns helps to avoid unintentionally including unwanted files, improving maintainability and reducing potential issues.

    8
    Remove redundant default settings for cleaner code

    The namespaces key in [tool.setuptools.packages.find] is set to false, which is the
    default value. Consider removing it for cleaner code unless you explicitly need to
    document this setting.

    py/pyproject.toml [7]

    -namespaces = false
    +# namespaces = false
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Removing redundant settings can make the code cleaner, but the impact on functionality is minimal since it is a default value.

    6
    Best practice
    Specify dependency versions to ensure compatibility

    Consider specifying the versions for the install_requires dependencies to ensure
    compatibility and prevent potential future conflicts.

    py/setup.py [59-60]

    -"urllib3[socks]>=1.26,<3",
    -"trio~=0.17",
    +"urllib3[socks]>=1.26,<1.27",
    +"trio~=0.17.0",
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Specifying exact versions for dependencies can help prevent compatibility issues, but the suggested versions may not be necessary if the current version constraints are sufficient.

    7

    @VietND96 VietND96 added the C-py Python Bindings label Oct 28, 2024
    @codecov
    Copy link

    codecov bot commented Oct 28, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 59.12%. Comparing base (215e20b) to head (523c2e3).
    Report is 4 commits behind head on trunk.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #14309      +/-   ##
    ==========================================
    + Coverage   59.09%   59.12%   +0.03%     
    ==========================================
      Files          91       91              
      Lines        5830     5835       +5     
      Branches      260      260              
    ==========================================
    + Hits         3445     3450       +5     
      Misses       2125     2125              
      Partials      260      260              

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    sandeepsuryaprasad commented Oct 29, 2024

    @VietND96 can you please re-trigger the workflow for this PR.. I have updated the branch.. Thanks!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    C-py Python Bindings P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants