Skip to content

refactor: introduced Diff and FileChange VOs#29

Merged
jordikroon merged 3 commits into
php:mainfrom
NickSdot:diff-value-objects
Jul 21, 2026
Merged

refactor: introduced Diff and FileChange VOs#29
jordikroon merged 3 commits into
php:mainfrom
NickSdot:diff-value-objects

Conversation

@NickSdot

Copy link
Copy Markdown
Contributor

Preflight. Adds two DTOs and extracts some logic from the sniffer. Not working with arrays made things much easier. Test are so that we have as little diff churn as possible and the assertions are goal-wise the same.

DiffParserTest has a small helper addition lineNumbersByFile() which I marked myself as todo; not optimal to merge todos but I need to keep things a bit organised if I want to get the fixer PR diff smaller. When fixers are merged I will follow up with the various todos.

@NickSdot

Copy link
Copy Markdown
Contributor Author

@jordikroon I had my first risky coverage CI failure here. Adding every class as an attribute, plus having its import, seems quite excessive. 😅 Are we fine with grouped attributes + FQN?

#[UsesClass(\DocbookCS\Source\File::class), UsesClass(\DocbookCS\Source\Line::class)]
final class SniffRunnerTest extends TestCase  
{
    /// ...
}

Comment thread src/Runner/SniffRunner.php
Comment thread src/Diff/Diff.php
@jordikroon

Copy link
Copy Markdown
Member

@jordikroon I had my first risky coverage CI failure here. Adding every class as an attribute, plus having its import, seems quite excessive. 😅 Are we fine with grouped attributes + FQN?

#[UsesClass(\DocbookCS\Source\File::class), UsesClass(\DocbookCS\Source\Line::class)]
final class SniffRunnerTest extends TestCase  
{
    /// ...
}

It is excessive and long, however I assume there is a high chance the class is used in the test directly. For that reason it's better to not use the FQNS. Otherwise a portion will be inconstant. Some IDE's do collapse the list of imported namespaces though.

@NickSdot
NickSdot force-pushed the diff-value-objects branch from 7b29346 to bdcf4e2 Compare July 21, 2026 08:06
@NickSdot

Copy link
Copy Markdown
Contributor Author

My bad that #30 and #31 didn't have the "depends on" in their description. Not merging them in order caused this one to conflict. Conflicts are resolved now, and the stacked 10 branches are rebased.

@jordikroon

Copy link
Copy Markdown
Member

Thanks. And as discussed you will handle the feedback in follow up MRs.

@jordikroon
jordikroon merged commit f8ebb86 into php:main Jul 21, 2026
3 checks passed
@NickSdot

Copy link
Copy Markdown
Contributor Author

Thanks. And as discussed you will handle the feedback in follow up MRs.

Yups. Both should be handled/obsolete. If I missed something let me know.

Will rebase now, then un-draft the next one.

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.

2 participants