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

Update pre-commits and pytest configuration #525

Merged
merged 12 commits into from
Jun 5, 2024
Merged

Conversation

lang-m
Copy link
Member

@lang-m lang-m commented Apr 29, 2024

User description

  • pre-commit: use ruff and ruff formatting to replace black, isort, flake8
  • pre-commit: remove hard-coded Python version
  • tests: convert all warnings to errors

Type

enhancement, bug_fix


Description

  • Updated pre-commit hooks and configurations to use newer versions and streamline settings.
  • Removed flake8 configurations from setup.cfg as the project transitions to using ruff for linting.
  • Enhanced Python project settings in pyproject.toml to handle specific false positives and improve test configurations.
  • Minor code and formatting improvements across various Python modules for better readability and maintenance.

Changes walkthrough

Relevant files
Configuration changes
3 files
.pre-commit-config.yaml
Update and streamline pre-commit configurations                   

.pre-commit-config.yaml

  • Updated versions for pre-commit hooks.
  • Removed unused configurations.
  • Added new hook configurations.
  • +2/-5     
    pyproject.toml
    Enhance Python project configurations                                       

    pyproject.toml

  • Added new configurations for tool.coverage.run and
    tool.pytest.ini_options.
  • Updated tool.ruff settings to handle notebook specific false
    positives.
  • +14/-7   
    setup.cfg
    Remove flake8 configurations                                                         

    setup.cfg

    • Removed all flake8 configurations.
    +0/-28   
    Enhancement
    1 files
    field.py
    Simplify dictionary comprehension in field.py                       

    discretisedfield/field.py

    • Simplified dictionary comprehension in _r_dim_mapping property.
    +1/-1     
    Formatting
    10 files
    __init__.py
    Minor formatting improvement in __init__.py                           

    discretisedfield/init.py

    • Added a blank line for better code organization.
    +1/-0     
    __init__.py
    Minor formatting improvement in io/__init__.py                     

    discretisedfield/io/init.py

    • Added a blank line for clarity.
    +1/-0     
    ovf2vtk.py
    Minor formatting improvement in ovf2vtk.py                             

    discretisedfield/io/ovf2vtk.py

    • Added a blank line for clarity.
    +1/-0     
    __init__.py
    Minor formatting improvement in plotting/__init__.py         

    discretisedfield/plotting/init.py

    • Added a blank line for clarity.
    +1/-0     
    hv.py
    Minor formatting improvement in hv.py                                       

    discretisedfield/plotting/hv.py

    • Added a blank line for clarity.
    +1/-0     
    k3d_field.py
    Minor formatting improvement in k3d_field.py                         

    discretisedfield/plotting/k3d_field.py

    • Added a blank line for clarity.
    +1/-0     
    mpl.py
    Minor formatting improvement in mpl.py                                     

    discretisedfield/plotting/mpl.py

    • Added a blank line for clarity.
    +1/-0     
    mpl_field.py
    Minor formatting improvement in mpl_field.py                         

    discretisedfield/plotting/mpl_field.py

    • Added a blank line for clarity.
    +1/-0     
    __init__.py
    Minor formatting improvement in tools/__init__.py               

    discretisedfield/tools/init.py

    • Added a blank line for clarity.
    +1/-0     
    tasks.py
    Minor formatting improvement in tasks.py                                 

    tasks.py

    • Added a blank line for clarity.
    +1/-0     

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

    lang-m and others added 2 commits April 29, 2024 21:53
    - pre-commit: use ruff and ruff formatting to replace black, isort, flake8
    - pre-commit: remove hard-coded Python version
    - tests: convert all warnings to errors
    Copy link
    Contributor

    PR Description updated to latest commit (20b7dff)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR includes a variety of changes across multiple configuration files and Python scripts. The changes involve updates to pre-commit hooks, removal of old linting configurations, and adjustments in testing configurations. Understanding the implications of these changes, especially in the context of continuous integration and code quality, requires a thorough review.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The change in discretisedfield/field.py modifies the behavior of the _r_dim_mapping method by removing the default return value of None for missing keys in the dictionary comprehension. This could lead to a KeyError if reversed_mapping does not contain a key found in self.mesh.region.dims.

    🔒 Security concerns

    No

    Code feedback:
    relevant filediscretisedfield/field.py
    suggestion      

    Consider restoring the default value of None in the dictionary comprehension to avoid potential KeyError. This can be done by modifying the line to include a default return value when the key is not found in the reversed_mapping. [important]

    relevant linereturn {dim: reversed_mapping.get(dim) for dim in self.mesh.region.dims}


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Bug
    Add a default value in dictionary lookup to prevent potential None values.

    In the _r_dim_mapping method, the reversed_mapping.get(dim) call should include a default
    value to handle cases where dim is not present in reversed_mapping. This prevents the
    method from returning None for some dimensions, which could lead to unexpected behavior or
    errors in other parts of the code that rely on this mapping.

    discretisedfield/field.py [604]

    -return {dim: reversed_mapping.get(dim) for dim in self.mesh.region.dims}
    +return {dim: reversed_mapping.get(dim, dim) for dim in self.mesh.region.dims}
     
    Enhancement
    Update the revision number in pre-commit configuration to the latest version.

    Update the rev version for the pre-commit/pre-commit-hooks to v4.5.0 to match the new
    version specified in the PR. This ensures that all hooks use the latest versions for
    consistency and to leverage any new features or bug fixes in the hooks.

    .pre-commit-config.yaml [5]

    -rev: v4.4.0
    +rev: v4.5.0
     
    Expand the list of ignored warnings in pytest configuration for clearer test outputs.

    It's beneficial to specify a more comprehensive list of warnings to ignore in the pytest
    configuration to avoid common false positives and focus on significant warnings during
    testing.

    pyproject.toml [112-115]

     filterwarnings = [
    +    "error",
    +    "ignore:DeprecationWarning",
    +    "ignore:UserWarning",
    +    "ignore:((.|\n)*)Sentinel is not a public part of the traitlets API((.|\n)*)"
    +]
     
    Best practice
    Refine the pattern for omitting files in coverage configuration to improve accuracy.

    The omit configuration in [tool.coverage.run] should include more specific patterns to
    ensure that only test files are omitted from coverage reports, rather than excluding all
    files under the tests directory. This change helps in maintaining accurate test coverage
    metrics.

    pyproject.toml [73]

    -omit = ["discretisedfield/tests/*"]
    +omit = ["discretisedfield/tests/**/*.py"]
     
    Reintroduce the trailing whitespace hook for cleaner code.

    Consider reintroducing the trailing-whitespace hook in the pre-commit configuration. This
    hook is useful for maintaining clean code by automatically removing unnecessary trailing
    whitespaces, which are generally considered bad practice in many coding standards.

    .pre-commit-config.yaml [7-10]

     hooks:
    +  - id: trailing-whitespace
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    discretisedfield/tools/tools.py Outdated Show resolved Hide resolved
    discretisedfield/tools/tools.py Outdated Show resolved Hide resolved
    @samjrholt samjrholt marked this pull request as ready for review June 5, 2024 09:44
    @samjrholt samjrholt merged commit 7ac627f into master Jun 5, 2024
    8 checks passed
    @samjrholt samjrholt deleted the precommit_updates branch June 5, 2024 10:45
    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.

    None yet

    3 participants