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

Expand references #156

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Expand references #156

wants to merge 37 commits into from

Conversation

vjik
Copy link
Member

@vjik vjik commented Dec 7, 2023

Q A
Is bugfix? ✔️
New feature? ❌
Breaks BC? ✔️/❌

Fix #153

@codecov Codecov
Copy link

codecov bot commented Dec 7, 2023 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8ef9884) 100.00% compared to head (68d3ad5) 100.00%.

Additional details and impacted files 
@@             Coverage Diff             @@
##              master      #156   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       277       277           
===========================================
  Files             22        23    +1     
  Lines            728       729    +1     
===========================================
+ Hits             728       729    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@what-the-diff What The Diff
Copy link

what-the-diff bot commented Dec 7, 2023

PR Summary

  • New File Addition
    A new file called MergePlanCollector.php was added.

  • Type Modification
    The type of class property $mergePlan was modified in the file src/Composer/MergePlanProcess.php. It moved from being MergePlan to MergePlanCollector, a newly introduced file.

  • Constructor Modification
    In the file src/Config.php, the constructor for MergePlan has been revised to use MergePlanCollector instead.

  • Method Relocation
    Some of the elements present in MergePlan class have been removed and relocated to MergePlanCollector.

  • Test Case Update
    In tests/Composer/TestCase.php, tests have been updated to reflect the newly added files in the assertMergePlan method. This ensures that our testing covers these new additions.

@vjik vjik added the status:under development Someone is working on a pull request. label Dec 10, 2023
@vjik vjik marked this pull request as ready for review December 10, 2023 15:05
samdark
samdark reviewed Dec 13, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
src/Composer/MergePlanCollector.php Outdated Show resolved Hide resolved
vjik and others added 6 commits December 19, 2023 16:23
@vjik vjik changed the title Expand variables Expand references Dec 23, 2023
samdark
samdark reviewed Dec 25, 2023
View reviewed changes
src/Composer/MergePlanCollector.php
}

/**
* Adds a multiple items to the merge plan.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Adds a multiple items to the merge plan.
* Adds multiple items to the merge plan.

src/Composer/MergePlanCollector.php
}

/**
* Add empty group if it doesn't exist.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Add empty group if it doesn't exist.
* Add an empty group if it doesn't exist.

src/Composer/MergePlanCollector.php
* @psalm-param list<FileType> $files
* @psalm-return list<FileType>
*/
private function replaceVariableToFiles(array $items, string $variable, array $files): array
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private function replaceVariableToFiles(array $items, string $variable, array $files): array
private function replaceVariableWithFiles(array $items, string $variable, array $files): array

src/Composer/MergePlanCollector.php
foreach ($groupPackages as $groupPackage => $groupItems) {
$packageItems = $packages[$groupPackage] ?? [];
$packages[$groupPackage] = in_array($variable, $packageItems, true)
? $this->replaceVariableToFiles($packageItems, $variable, $groupItems)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? $this->replaceVariableToFiles($packageItems, $variable, $groupItems)
? $this->replaceVariableWithFiles($packageItems, $variable, $groupItems)

src/FilesExtractor.php
$this->throwException(sprintf('The "%s" file does not found.', $file));
$message = Options::isVariable($item)
? sprintf('Don\'t allow to use variables in environments. Found variable "%s".', $item)
: sprintf('The "%s" file does not found.', $file);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: sprintf('The "%s" file does not found.', $file);
: sprintf('The "%s" file is not found.', $file);

src/FilesExtractor.php
} elseif (!$isOptional) {
$this->throwException(sprintf('The "%s" file does not found.', $file));
$message = Options::isVariable($item)
? sprintf('Don\'t allow to use variables in environments. Found variable "%s".', $item)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? sprintf('Don\'t allow to use variables in environments. Found variable "%s".', $item)
? sprintf('Can\'t use variables in environments. Found variable "%s".', $item)

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

@samdark samdark samdark left review comments

Assignees
No one assigned
Labels
status:under development Someone is working on a pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Throw duplicate error on use nested groups
3 participants
@vjik @samdark @StyleCIBot