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

[PLA-1678] Laravel 11 update. #129

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

v16Studios
Copy link
Contributor

@v16Studios v16Studios commented Jun 24, 2024

PR Type

enhancement, configuration changes


Description

  • Removed the PHP CS Fixer configuration file.
  • Updated composer.json to require PHP "^8.2" and replaced friendsofphp/php-cs-fixer with laravel/pint.
  • Added a new script in composer.json for fix using laravel/pint.
  • Added a new configuration file pint.json for Laravel Pint with extensive rules.

Changes walkthrough 📝

Relevant files
Configuration changes
.php-cs-fixer.php
Remove PHP CS Fixer configuration file                                     

.php-cs-fixer.php

  • Removed the PHP CS Fixer configuration file.
+0/-165 
pint.json
Add Laravel Pint configuration file                                           

pint.json

  • Added a new configuration file for Laravel Pint with extensive rules.
  • +175/-0 
    Enhancement
    composer.json
    Update dependencies and scripts in composer.json                 

    composer.json

  • Updated PHP requirement to "^8.2".
  • Replaced friendsofphp/php-cs-fixer with laravel/pint.
  • Added a new script for fix using laravel/pint.
  • +3/-2     

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Configuration Consistency:
    Ensure that the new pint.json configuration aligns with the project's coding standards and that all necessary rules are included as per the removed .php-cs-fixer.php.
    Dependency Update:
    Verify that the new dependency laravel/pint is compatible with other project dependencies and the Laravel 11 framework.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Broaden the PHP version compatibility to include more users

    Consider specifying a broader range of compatible PHP versions if possible. Restricting to
    "^8.2" might limit users who are on slightly older but still supported versions of PHP.

    composer.json [25]

    -"php": "^8.2",
    +"php": "^8.1 || ^8.2",
     
    Suggestion importance[1-10]: 9

    Why: Broadening the PHP version compatibility can help include users who are on slightly older but still supported versions, increasing the usability of the package.

    9
    Maintainability
    Ensure continuity of code style checks by adding back or replacing necessary packages

    Add a replacement for the removed 'friendsofphp/php-cs-fixer' in 'require-dev' to maintain
    code style checks or clarify the transition strategy to 'laravel/pint'.

    composer.json [31]

    -"laravel/pint": "^1.16"
    +"laravel/pint": "^1.16",
    +"friendsofphp/php-cs-fixer": "^3.0"
     
    Suggestion importance[1-10]: 8

    Why: Adding back 'friendsofphp/php-cs-fixer' or clarifying the transition to 'laravel/pint' ensures that code style checks remain consistent and maintainable.

    8
    Best practice
    Add a 'lint' script for consistency and clarity in usage

    Consider adding a script for 'lint' that uses 'laravel/pint' for consistency with typical
    Laravel project setups.

    composer.json [40]

    +"lint": "vendor/bin/pint",
     "fix": "vendor/bin/pint",
     
    Suggestion importance[1-10]: 7

    Why: Adding a 'lint' script aligns with typical Laravel project setups and provides clarity in usage, although it is a minor enhancement.

    7
    Possible issue
    Adjust a linting rule to potentially avoid conflicts with default argument values

    Review the 'no_unreachable_default_argument_value' rule to ensure it does not interfere
    with intended default behaviors in function arguments.

    pint.json [72]

    -"no_unreachable_default_argument_value": true,
    +"no_unreachable_default_argument_value": false,
     
    Suggestion importance[1-10]: 6

    Why: Reviewing and potentially adjusting the 'no_unreachable_default_argument_value' rule can prevent unintended conflicts, though it may not be necessary for all projects.

    6

    @v16Studios v16Studios requested a review from zlayine June 24, 2024 22:26
    @leonardocustodio leonardocustodio self-requested a review June 26, 2024 00:12
    @leonardocustodio
    Copy link
    Member

    @v16Studios commits are not signed

    @v16Studios v16Studios force-pushed the update/pla-1678/laravel-11-update branch from fdf21e2 to e570646 Compare June 26, 2024 00:25
    @v16Studios v16Studios merged commit ffa0c52 into master Jun 26, 2024
    3 checks passed
    @v16Studios v16Studios deleted the update/pla-1678/laravel-11-update branch June 26, 2024 00:32
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants