Skip to content

[12.x] reapply Pint style changes #55015

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

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

browner12
Copy link
Contributor

@taylorotwell please do not merge right away. in this final Pint fix, we have a conflict with StyleCI, and I'm not sure if there's a way to adjust the rules to get them to play nice, without StyleCI immediately reverting the changes.

we're in a little bit of a chicken/egg situation due to StyleCIs rules, which seems to have some inconsistencies on if it wants opening braces for an anonymous class on a newline or the same line.

I think we've reached to point where we need to decide if we're dropping StyleCI in favor of Pint, which would be my vote.

However, prior to doing that, we would need to setup Pint in the Github Actions pipeline, which I believe would probably need to be handled by the Laravel team. As discussed in a previous PR, we have 2 options for using Pint in our CI.

  • we can use pint --test to either pass or fail the PR, and force the contributor to ensure style rules are followed. they can do this manually, or run pint to automatically apply fixes. (my vote)
  • we could also use regular pint in CI and have it auto-commit changes

below is a simple Github Action job I've used for projects before:

style:
    name: 'Coding Style'
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Setup PHP
        uses: shivammathur/setup-php@v2
        with:
          php-version: ${{ matrix.php }}
      - name: Install Composer Dependencies
        run: composer install --no-progress --prefer-dist --optimize-autoloader
      - name: Run Laravel Pint
        run: ./vendor/bin/pint --test -v

StyleCI reverted all the changes from the previous commit.
Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

I feel that this is the wrong approach, and would not recommend merging this. Instead, we should adjust the StyleCI preset and remove Pint from here.

@timacdonald
Copy link
Member

timacdonald commented Mar 13, 2025

I have no opinion on StyleCI vs Pint, however if we were to move forward with Pint, we should configure the action to run on push to x.x and master branches and auto commit changes.

That way it does not become a blocker for merging PRs. Running --test on PRs will be small, but annoying, blocker to contributing to the project.

It would be best if it ran in the same task as facade documenter as the following step, that way there is now way they should start an infinite loop, which we have seen in the past with StyleCI.

This matches how StyleCI and the Facade documenter get out of the way.

@taylorotwell taylorotwell merged commit 5b661de into laravel:12.x Mar 14, 2025
40 of 41 checks passed
@taylorotwell
Copy link
Member

I honestly have no strong feelings about this whole Pint or StyleCI thing. I would just rather focus on useful features for end users.

@browner12 browner12 deleted the AB-pint-one-more-time branch March 14, 2025 17:27
@browner12
Copy link
Contributor Author

yes, formatting is not the most glamorous thing, but I think it's important for long term maintainability of a project, and encouraging good contributions.

Pint seems to be more powerful than StyleCI, which is why I think it should be our only tool, and that also seemed to get decent community support in the first PR I wrote.

@GrahamCampbell
Copy link
Member

StyleCI is more powerful than Pint, and is much faster than running Pint on actions.

@browner12
Copy link
Contributor Author

I don't know that either of those comments are true. Again, the whole reason this started is because there were rules I could do in Pint that I could not do in StyleCI. And most of my projects run their Pint CI in like 20 seconds or less, so even if StyleCI is slightly faster than Pint, they're both insanely faster than any testing pipeline jobs we run.

@AhmedAlaa4611
Copy link
Contributor

I love to use the same styling tool for developing my project as well as for contributing to the Laravel core itself, we need Pint

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants