-
-
Notifications
You must be signed in to change notification settings - Fork 135
feat!: bump default formatting version to composer version with fallaback to latest supported PHP #2434
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
Conversation
a81d0fe
to
1f20ced
Compare
Just an idea, should we have a guess at this from the composer.json file at the project root? or is that too complicated? Look for ...
"require": {
"php": ">=8.2",
}
... |
@cseufert Sounds reasonable! I don't have much experience with composer - does one typically set the version constraint to be equal or above the PHP version that is actually in use? If yes, this would probably make sense. We could then implement a logic like this:
If this logic makes sense, we might still want to merge this PR (just to keep the fallback to a realistic value). |
|
I have added basic detection for min php version from composer.json package. Should work for most cases, falls back to 8.3 as default if composer.json not found, or invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor things, nice work!!
@czosel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR bumps the default PHP formatting target to 8.3, introduces automatic version detection via composer.json
, and updates snapshots/tests to explicitly include version and trailing commas.
- Added
getComposerPhpVer
utility and setphpVersion
default togetComposerPhpVer() ?? "8.3"
, plus added"8.4"
to the choices. - Updated
run_spec
invocations and Jest snapshots across many test suites to include explicitphpVersion: "7.0"
and trailing commas. - Expanded Jest’s
testRegex
to match all.spec.js
/.spec.mjs
files and added new tests forgetComposerPhpVer
.
Reviewed Changes
Copilot reviewed 46 out of 47 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/options.mjs | Implements dynamic PHP version detection and bumps fallback to 8.3; adds 8.4 choice |
jest.config.mjs | Broadens test matching pattern to .*\.spec\.m?js$ |
tests/composer-version/composer-version.spec.mjs | New tests covering getComposerPhpVer behavior |
tests/trailing_commas/jsfmt.spec.mjs | Adds explicit phpVersion to formatting specs |
Comments suppressed due to low confidence (2)
src/options.mjs:70
- Consider adding a unit test to verify that when no
composer.json
is found (or parsing fails),getComposerPhpVer()
returns null and the default falls back to "8.3" as intended.
default: getComposerPhpVer() ?? "8.3",
src/options.mjs:10
- [nitpick] The function name
getComposerPhpVer
could be expanded togetComposerPhpVersion
for clarity and consistency with thephpVersion
option name.
function getComposerPhpVer() {
Apart from the nitpick this looks good to me now 👍 |
I have updated the docs, however thinking about it further, maybe |
Interesting idea, that would make it easier to debug when the composer lookup is not working (instead of the silent fallback to Another idea would be to have two "special" settings:
What do you think? Too much or just right? 🙃 |
a182b2b
to
469d8fe
Compare
BREAKING CHANGE: If you didn't set the `phpVersion` option explicitly, the formatting will assume PHP 8.3 now (instead of 7.0).
- added wildcard major versions - updated test for null return - added invalid json test case
- automatically use latest version - phpVersion is always a number now - moved isMinVersion to simple number comparison
469d8fe
to
e8b5ea8
Compare
I have re-implemented the default version, check out the readme, but in essence Apologies, as this has turned into a rather large PR though now, as multiple things have changed at once. Lines 191 to 205 in 1ea1177
|
@cseufert Looks great!! 💯 A few very small cleanups, then we're good to go 🚀 |
Co-authored-by: Christian Zosel <[email protected]>
Co-authored-by: Christian Zosel <[email protected]>
Co-authored-by: Christian Zosel <[email protected]>
All good now! Thanks @cseufert 💯 |
Released in |
BREAKING CHANGE: If you didn't set the
phpVersion
option explicitly, the formatting will try and located the minimum supported version from your composer.json (require.php
), else the latest supported PHP version (currently: 8.4) now (instead of 7.0).As suggested in #2396 by @mreiden