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

chore: IntegrationTest - move support of php< requirement to main Integration classes #7448

Merged
merged 7 commits into from
Nov 20, 2023

Conversation

keradus
Copy link
Member

@keradus keradus commented Nov 20, 2023

No description provided.

@keradus keradus marked this pull request as ready for review November 20, 2023 13:52
@coveralls
Copy link

coveralls commented Nov 20, 2023

Coverage Status

coverage: 94.785%. remained the same
when pulling 97588be on keradus:php_
into b090e9d on PHP-CS-Fixer:master.

Comment on lines 61 to 63
* * Section or any line in it may be omitted.
* ** PHP minimum version. Default to current running php version (no effect).
* *** PHP upper limit, that is first too-high version. Default is empty (no effect).
Copy link
Member

@Wirone Wirone Nov 20, 2023

Choose a reason for hiding this comment

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

Using asterisk here is IMHO totally unreadable, because whole phpDoc is behind *. Single asterisk after section marker can't be there in actual test case, also ** and *** makes JSON invalid. I would totally change it since you're cleaning this up.

Suggested change
* * Section or any line in it may be omitted.
* ** PHP minimum version. Default to current running php version (no effect).
* *** PHP upper limit, that is first too-high version. Default is empty (no effect).
* IMPORTANT:
* - Some sections (like `--CONFIG--`) may be omitted. The required sections are: `--TEST--`, `--RULESET--` and `--EXPECT--` (which works as input too if `--INPUT--` is not provided, that means no changes are expected).
* - `--REQUIREMENTS--` section can define additional constraints for running (or not) the test. You can use these fields to fine-tune run conditions for test cases:
* - `php` represents minimum PHP version test should be run on. Default to current running PHP version (no effect).
* - `php<` represents maximum PHP version test should be run on. Default to PHP's maximum integer value (no effect).
* - `os` represents operating system(s) test should be run on. Supported operating systems are Linux, Darwin and Windows. By default test is run on all supported operating systems.

The asterisks must be removed from the snippet, but I can't make full suggestion here.

Copy link
Member Author

@keradus keradus Nov 20, 2023

Choose a reason for hiding this comment

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

I'm not in favour of manually listing which section is obligatory instead marking it in format itself.
if the full suggestion is not working here for you, maybe raise a dedicated PR/subPR?

Copy link
Member

Choose a reason for hiding this comment

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

But it makes format malformed and unclear, since asterisks are either on the left (phpDoc) and on the right ("annotations"). Your choice 🤷‍♂️.

Copy link
Member Author

@keradus keradus Nov 21, 2023

Choose a reason for hiding this comment

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

I extended the already existing list. If you want to change syntax of this list, please raise a new PR (especially as you had a code in mind, just you couldn't provide full snippet, as you said).

For me, out of scope of this PR, following your reasoning from previous PRs :D

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough 😉.

@keradus keradus merged commit 6cf20e4 into PHP-CS-Fixer:master Nov 20, 2023
23 checks passed
@keradus keradus deleted the php_ branch November 20, 2023 21:57
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.

None yet

3 participants