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

Sort allow-plugins and preferred-install when feasible #980

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

fredden
Copy link
Contributor

@fredden fredden commented Oct 3, 2023

Following on from the discussion in #775 (comment), this pull request re-adds sorting of allow-plugins and preferred-install, but takes into account the difficult case of a wildcard in the middle of a package name.

This pull request

  • Sorts config.allow-plugins in composer.json when there are no wildcards used.
  • Sorts config.allow-plugins in composer.json when there are wildcards used, but only at the end of package names.
  • Leaves config.allow-plugins in composer.json unsorted when there are wildcards used in the middle of a package name.
  • Sorts config.preferred-install in composer.json when there are no wildcards used.
  • Sorts config.preferred-install in composer.json when there are wildcards used, but only at the end of package names.
  • Leaves config.preferred-install in composer.json unsorted when there are wildcards used in the middle of a package name.

Follows #775 (comment)

I considered only sorting the packages when there are no wildcards. This would make the code much more simple, and remove any chance of messing up the wildcard parsing process. I opted to sort the list whenever possible, including when wildcards are used, but kept the not-sorted behaviour when a wildcard is found part-way-through a package name.

Another approach that I considered was to remove entries which are unreachable (see the '.../IsUseful/No/' test fixtures). I decided to keep these as users may have added entries thinking that the list sorts the other way (last match, whereas it is actually first-match). Correcting this seems most useful to end users. They can then make their own decision if the entry should exist or not.

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #980 (45fc200) into main (318079a) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #980      +/-   ##
============================================
+ Coverage     97.52%   97.62%   +0.09%     
- Complexity      166      173       +7     
============================================
  Files            30       30              
  Lines           728      758      +30     
============================================
+ Hits            710      740      +30     
  Misses           18       18              
Files Coverage Δ
src/Vendor/Composer/ConfigHashNormalizer.php 95.83% <100.00%> (+6.94%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fredden fredden force-pushed the composer/config/allow-plugins branch 2 times, most recently from 3701c8c to 342bbae Compare October 9, 2023 14:22
@localheinz localheinz self-assigned this Oct 10, 2023
@localheinz localheinz force-pushed the composer/config/allow-plugins branch from 342bbae to 9d8f548 Compare October 10, 2023 08:58
@localheinz localheinz changed the title Sort allow-plugins and preferred-install when feasible Sort allow-plugins and preferred-install when feasible Oct 10, 2023
@localheinz localheinz force-pushed the composer/config/allow-plugins branch from 9d8f548 to 45fc200 Compare October 10, 2023 09:54
No sorting will be applied if there are wildcards in the middle of strings.
@localheinz localheinz force-pushed the composer/config/allow-plugins branch from 45fc200 to 31e7029 Compare October 10, 2023 09:58
@localheinz localheinz merged commit 5a926b5 into ergebnis:main Oct 10, 2023
13 checks passed
@localheinz
Copy link
Member

Thank you, @fredden!

@fredden fredden deleted the composer/config/allow-plugins branch October 10, 2023 09:59
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.

2 participants