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

DX: Introduce Composer scripts as common DX #6839

Merged
merged 11 commits into from
Aug 5, 2023

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Mar 14, 2023

The goal of this change is to provide common entrypoints for dev tasks required during development. This way we can ensure everybody runs the same command, and we can change underlying commands without changing dev flow (Composer scripts' names are our contract).

I have doubts about some of these commands (e.g. scripts included in dev-tools:check), but we can clean this in separate PR after some discussion. At this point I just migrated existing calls to Composer scripts.

Usage

Usage is straightforward: scripts are called via composer, for example composer qa will run QA suite.

Running tests

In terms of tests there are 3 scripts and their intention is rather clear. However I would like to emphasise that test runs all tests (basically it's a proxy to vendor/bin/paraunit run), but it's still possible to limit tests execution just like in regular ParaUnit call, the only thing is that -- must be used between Composer script and the list of arguments passed to actual script, like:

$ c test -- tests/Fixer/Alias/ArrayPushFixerTest.php                                        
> Composer\Config::disableProcessTimeout
> paraunit run --testsuite all 'tests/Fixer/Alias/ArrayPushFixerTest.php'

PARAUNIT v.1.3.0
by Francesco Panina, Alessandro Lai & Shark Dev Team @ Facile.it
..........................................SS........                          52

Execution time -- 00:00:00

Executed: 1 test classes, 52 tests

1 files with SKIPPED OUTCOME:
  PhpCsFixer\Tests\Fixer\Alias\ArrayPushFixerTest

CONTRIBUTING.md Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
composer.json Show resolved Hide resolved
@Wirone Wirone changed the title Introduce Composer scripts as common DX DX: Introduce Composer scripts as common DX Mar 14, 2023
@coveralls
Copy link

coveralls commented Mar 14, 2023

Pull Request Test Coverage Report for Build 5771227596

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.607%

Totals Coverage Status
Change from base Build 5771098170: 0.0%
Covered Lines: 24189
Relevant Lines: 25568

💛 - Coveralls

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@Wirone Wirone requested a review from keradus March 22, 2023 14:32
Copy link
Contributor

@kubawerlos kubawerlos left a comment

Choose a reason for hiding this comment

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

Can we for a start go with only scripts that will be used in README.md, CONTRIBUTING.md and GitHub Actions?

@Wirone
Copy link
Member Author

Wirone commented Mar 22, 2023

@kubawerlos I don't understand why? This PR is a proposal for unified way of invoking dev-related scripts, I want to use them while working on Fixer and from my experience it's good when there's common DX for working with the project. When you have such scripts you can change them under the hood while entry point remains the same. Which scripts you would like to remove for now?

@kubawerlos
Copy link
Contributor

@Wirone

This PR is a proposal for unified way of invoking dev-related scripts, I want to use them while working on Fixer

Is it to have in unified way or in your way?

from my experience it's good when there's common DX for working with the project.

No doubt about it is useful to have some shortcuts, yet I believe there is a reason why there are no such shortcuts in the repo yet. @keradus already mentioned Makefile, I have quite different habits for scripts naming, other people will have other habits too. It is easy when working a full-time job on a codebase with a limited number of developers - fewer people, all focused on the repository/repositories, many hours per day. It is a whole different story in open source. I'm not going to use those scripts because memorizing them is a much bigger effort than profit of (rarely) using them - and I don't believe I'm unique in that thinking.

@Wirone
Copy link
Member Author

Wirone commented Mar 23, 2023

Is it to have in unified way or in your way?

It is based on my experience and of course may be subjective, but general idea is universal - having common, simple entrypoints for dev actions, that can change under the hood over time (options, arguments, converting single command to group of commands etc.) without changing the entrypoint itself.

I believe there is a reason why there are no such shortcuts in the repo yet

Just like proper Dependency Injection, yet we're considering introducing it, right? 😉

I'm not going to use those scripts because memorizing them is a much bigger effort than profit of (rarely) using them

The good thing is that you really don't need to memorise everything. Just run composer and you'll get list of available commands 🙂. For me having such set of commands is a basic DX because most of those actions are used every time when developing.

It's also easier to maintain because, as mentioned, you can include those commands in documentation and they rather won't change, while you can tweak what's under the hood.

I believe the assumption that people won't use it is not an argument against merging it. In the same way probably people don't use Docker setup which is included in the repo, but it's here. Since those commands are in the contributing guideline, I think many people will start to use them, I don't know why my assumption should be less important 😉.

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@Wirone Wirone force-pushed the codito/improve-dx branch 2 times, most recently from 76703ef to a040380 Compare July 10, 2023 14:07
@Wirone Wirone requested a review from kubawerlos July 10, 2023 14:25
@Wirone Wirone self-assigned this Jul 10, 2023
@Wirone
Copy link
Member Author

Wirone commented Jul 11, 2023

@keradus ⏳ 👀

@Wirone Wirone added the RTM Ready To Merge label Jul 11, 2023
CONTRIBUTING.md Outdated Show resolved Hide resolved
Always analyse and fix whole codebase (focus on built-in cache mechanism).
Since ParaUnit was introduced (after rebase I've changed underlying commands) I believe we can drop `test:branch` and `test:diff` completely. With fast linter `composer test` takes about 30s, which is totally fine to run every time before commit/push.
@Wirone
Copy link
Member Author

Wirone commented Aug 5, 2023

I am going to merge this, because all discussions were resolved, it has approval and I just want to use these scripts (current approach drives me mad). If @keradus decides it should be reverted or improved, we can do it later before next release.

@Wirone Wirone merged commit f3f35bd into PHP-CS-Fixer:master Aug 5, 2023
14 checks passed
@Wirone Wirone deleted the codito/improve-dx branch August 5, 2023 14:44
composer.json Show resolved Hide resolved
composer.json Show resolved Hide resolved
composer.json Show resolved Hide resolved
@keradus keradus removed the RTM Ready To Merge label Sep 18, 2023
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

5 participants