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.

Sign up for GitHub

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

Jump to bottom

Rename fixtures paths to be consistent #52169

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Rename fixtures paths to be consistent #52169

merged 1 commit into from
Oct 20, 2023

Conversation

smnandre
Copy link
Member

Q A
Branch? 6.4
Bug fix? no (maybe)
New feature? no
Deprecations? no
License MIT

There are currently two components with a "fixtures" (lowercase) folder,

So on case-sensitive filesystems the following exclusion in php-cs-fixer does not work.

(new PhpCsFixer\Finder())
    ->in(__DIR__.'/src')
    ->append([__FILE__])
    ->notPath('#/Fixtures/#')

Those are the files who are currently not excluded on case-sensitive filesystems

php-cs-fixer list-files | grep fixtures
'./src/Symfony/Component/Translation/Tests/fixtures/extractor-ast/translation.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor-ast/translatable-short.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor-ast/translatable-fqn.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor-ast/translatable.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor-ast/validator-constraints.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor/translation.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor/translatable-short.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor/translatable-fqn.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor/translatable.html.php'
'./src/Symfony/Component/Translation/Tests/fixtures/extractor-7.3/translation.html.php'
'./src/Symfony/Component/AssetMapper/Tests/fixtures/importmap.php'
'./src/Symfony/Component/AssetMapper/Tests/fixtures/importmaps/importmap.php'
'./src/Symfony/Component/AssetMapper/Tests/fixtures/AssetMapperTestAppKernel.php'

This PR exclude files in both senstivie / insenstive systems.

@carsonbot carsonbot added Status: Needs Review DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Oct 19, 2023
@carsonbot carsonbot added this to the 6.4 milestone Oct 19, 2023
@carsonbot carsonbot changed the title [DX] Make '#/Fixtures/#' exclusion pattern case-incensitive Make '#/Fixtures/#' exclusion pattern case-incensitive Oct 19, 2023
@OskarStark
Copy link
Contributor

What about renaming those folders?

@smnandre
Copy link
Member Author

What about renaming those folders?

I didn't dare to suggest. Really.

@smnandre smnandre force-pushed the fix/php-cs-fixtures-case branch from cc49c65 to 23469b0 Compare October 19, 2023 18:21
@smnandre
Copy link
Member Author

@OskarStark there is the "rename" option. What do you think ?

(if this pass, all in-progress PR will probably hate me)

fabbot fails because there is an invalid json file (the same there was but the path has changed...)

@OskarStark
Copy link
Contributor

Don't worry, they can rebase their PRs

@OskarStark OskarStark requested a review from weaverryan October 20, 2023 04:17
@OskarStark OskarStark changed the title Make '#/Fixtures/#' exclusion pattern case-incensitive Rename fixtures paths to be consistent Oct 20, 2023
@fabpot fabpot force-pushed the fix/php-cs-fixtures-case branch from 5b07a61 to ec09b40 Compare October 20, 2023 05:44
@fabpot
Copy link
Member

fabpot commented Oct 20, 2023

Thank you @smnandre.

@fabpot fabpot merged commit 3bfb8c2 into symfony:6.4 Oct 20, 2023
5 of 7 checks passed
keradus
keradus reviewed Oct 27, 2023
View reviewed changes
src/Symfony/Component/Translation/Tests/Loader/PoFileLoaderTest.php
@@ -110,7 +110,7 @@ public function testSkipFuzzyTranslations()
public function testMissingPlurals()
{
$loader = new PoFileLoader();
$resource = __DIR__.'/../fixtures/missing-plurals.po';
$resource = __DIR__ . '/../Fixtures/missing-plurals.po';
Copy link
Member

Choose a reason for hiding this comment

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

this is against the Fixer config and caused lot of extra output to PHP CS Fixer results.
let's fix it.

Copy link
Member

Choose a reason for hiding this comment

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

#52317

@xabbuh xabbuh mentioned this pull request Jan 23, 2024
Merged
xabbuh added a commit that referenced this pull request Jan 23, 2024
@xabbuh
minor #53618 [AssetMapper] fix tests (xabbuh)
13f326d 
This PR was merged into the 6.4 branch.

Discussion
----------

[AssetMapper] fix tests

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

Move the new directory introduced in #53260 to the `Fixtures` directory (`fixtures` had been renamed in #52169).

Commits
-------

48cf213 fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keradus keradus keradus left review comments

@OskarStark OskarStark OskarStark approved these changes

@fabpot fabpot fabpot approved these changes

@lyrixx lyrixx Awaiting requested review from lyrixx lyrixx is a code owner

@welcoMattic welcoMattic Awaiting requested review from welcoMattic welcoMattic is a code owner

@weaverryan weaverryan Awaiting requested review from weaverryan

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
5 participants
@smnandre @OskarStark @fabpot @keradus @carsonbot