Skip to content

Conversation

@sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Jul 12, 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

Moved all settings from setup.cfg file to pyproject.toml.

Motivation and Context

  • This eliminates the need to maintain setup.cfg file since all the settings are now moved to 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

  • Moved Flake8 and pytest configuration settings from setup.cfg to pyproject.toml.
  • Updated Bazel build configuration to reference pyproject.toml instead of setup.cfg.

Changes walkthrough 📝

Relevant files
Configuration changes
BUILD.bazel
Update Bazel build configuration to use `pyproject.toml` 

py/BUILD.bazel

  • Replaced setup.cfg with pyproject.toml in the data section.
+1/-1     
setup.cfg
Remove configuration settings from `setup.cfg`                     

py/setup.cfg

  • Removed Flake8 and pytest configuration settings.
+0/-11   
Enhancement
pyproject.toml
Add Flake8 configuration to `pyproject.toml`                         

py/pyproject.toml

  • Added Flake8 configuration settings.
+7/-0     

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

@qodo-merge-pro
Copy link
Contributor

PR Reviewer Guide 🔍

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

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Jul 12, 2024

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Maintainability
Remove redundant configuration settings

Remove the redundant setting max-line-length = 120 since E501 is ignored in
extend-ignore. This setting does not affect the behavior and could lead to
confusion.

py/pyproject.toml [10]

-max-line-length = 120
+# max-line-length = 120
 
  • Apply this suggestion
Suggestion importance[1-10]: 9

Why: The suggestion correctly identifies a redundant configuration setting that could cause confusion, improving maintainability and clarity of the configuration file.

9
Best practice
Add explanatory comments for specific lint ignores

Consider adding a comment explaining why E203 is included in extend-ignore,
especially if it relates to specific style or compatibility issues with other tools
like black.

py/pyproject.toml [8]

-extend-ignore = "E501, E203"
+extend-ignore = "E501, E203"  # E203 ignored due to whitespace before ':' which is fine in black
 
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Adding explanatory comments for specific lint ignores enhances code readability and helps maintainers understand the rationale behind certain configurations.

8
Ensure consistent exclusion of all virtual environment directories

*Since .tox and venv are excluded, ensure that all virtual environment patterns used
in the project (like .venv, env, etc.) are consistently excluded to avoid linting of
virtual environment directories.

py/pyproject.toml [6]

-exclude = ".tox,docs/source/conf.py,*venv"
+exclude = ".tox,docs/source/conf.py,*venv,env,.venv"
 
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Ensuring consistent exclusion of all virtual environment directories is a best practice that prevents unnecessary linting and potential issues, improving the overall development workflow.

8
Possible issue
Verify configuration migration completeness

Ensure that the removal of setup.cfg from the data list is intentional and that all
configurations are properly migrated to pyproject.toml to avoid runtime or
build-time issues.

py/BUILD.bazel [336]

-"setup.cfg",
+# "setup.cfg",  # Ensure all configurations are migrated to pyproject.toml
 
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion is valid as it prompts the developer to verify that all necessary configurations have been migrated, which is crucial for avoiding potential issues. However, it could be seen as more of a reminder than a code improvement.

7

@VietND96 VietND96 added the C-py Python Bindings label Nov 1, 2024
@VietND96
Copy link
Member

Is this still valid? Can you please resolve the conflicts

@sandeepsuryaprasad
Copy link
Contributor Author

sandeepsuryaprasad commented Nov 12, 2024

@VietND96 flake8 does not support pyproject.toml. Looks like we have to install 3rd party dependency pyproject-flake8 in-order for flake8 to take config settings from pyproject.toml. Since, setup.cfg file now has only flake8 settings, we can move this settings to tox.ini file. Flake8 documentation says below,

Flake8 supports storing its configuration in your project in one of setup.cfg, tox.ini, or .flake8.

So instead of having both setup.cfg and tox.ini, we i will move flake8 settings to tox.ini

@sandeepsuryaprasad
Copy link
Contributor Author

I will do the necessary changes to the current PR

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]: 2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants