Skip to content

Conversation

@Glancu
Copy link

@Glancu Glancu commented Oct 27, 2025

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

@loic425
Copy link
Member

loic425 commented Oct 27, 2025

Please split into 2 commits:

  • first one with only the changes on the same file
  • second one with the files moved and renamed.

@GSadee
Copy link
Member

GSadee commented Oct 28, 2025

Please split into 2 commits:

  • first one with only the changes on the same file
  • second one with the files moved and renamed.

Hi @loic425! Just to better understand your reasoning, could you explain why this level of separation into two commits is important in your view? From my perspective, it feels a bit excessive in this particular case, especially since the PR is already quite granular and focused only on specific directory. I'm curious what concrete benefits you see in splitting it this way. 🤔

@loic425
Copy link
Member

loic425 commented Oct 28, 2025

When splitting into 2 commits we can review the diff in the first commit.
With @diimpp, we love this way, it's way faster to review.

Look at the current PRs inside the Grid package, and you'll see the diff.

On my OpenCode, I've split this into two tasks.

@Glancu Glancu force-pushed the Glancuphpspec-to-phpunit-dependency-injection branch from 0864af4 to cf63259 Compare October 30, 2025 11:00
@Glancu Glancu force-pushed the Glancuphpspec-to-phpunit-dependency-injection branch from cf63259 to dfaa96c Compare October 30, 2025 13:29
…ncy-injection' into Glancuphpspec-to-phpunit-dependency-injection

# Conflicts:
#	src/Bundle/spec/DependencyInjection/Compiler/Helper/TargetEntitiesResolverSpec.php
#	src/Bundle/spec/DependencyInjection/Driver/Exception/InvalidDriverExceptionSpec.php
#	src/Bundle/spec/DependencyInjection/Driver/Exception/UnknownDriverExceptionSpec.php
@Glancu Glancu force-pushed the Glancuphpspec-to-phpunit-dependency-injection branch from 6f5323d to cf63259 Compare October 31, 2025 06:52
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.

3 participants