diff icon indicating copy to clipboard operation
diff copied to clipboard

Add ability to set contextLines in __construct() on UnifiedDiffOutputBuilder

Open samsonasik opened this issue 2 years ago • 7 comments

samsonasik avatar Dec 17 '23 07:12 samsonasik

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (654bd13) 99.32% compared to head (f3b2657) 99.32%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #122   +/-   ##
=========================================
  Coverage     99.32%   99.32%           
  Complexity      220      220           
=========================================
  Files            12       12           
  Lines           589      590    +1     
=========================================
+ Hits            585      586    +1     
  Misses            4        4           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 17 '23 08:12 codecov[bot]

I fixed psalm notice by add @psalm-param positive-int $contextLines https://github.com/sebastianbergmann/diff/pull/122/commits/f3b2657616ad280548ea574695886ec519640646

samsonasik avatar Dec 17 '23 08:12 samsonasik

Hi and thanks for the PR!

May I ask why you do not use the StrictUnifiedDiffOutputBuilder as it offers this configuration OOTB? See for example: https://github.com/sebastianbergmann/diff/blob/5.0.3/src/Output/StrictUnifiedDiffOutputBuilder.php#L39

SpacePossum avatar Dec 17 '23 16:12 SpacePossum

we use it directly on rector, see

https://github.com/rectorphp/rector-src/blob/affdec96db31156ab2b05a5801f980e284e291a5/src/Console/Formatter/CompleteUnifiedDiffOutputBuilderFactory.php#L27

It require private accessor to change that.

samsonasik avatar Dec 17 '23 16:12 samsonasik

I think maybe changing the output builder from UnifiedDiffOutputBuilder to StrictUnifiedDiffOutputBuilder in rector would be more beneficial. Using the strict output comes with all the benefits as the non-strict one and with even more benefits like proper-udiff format and more configuration options like the one you want to add here.

SpacePossum avatar Dec 17 '23 16:12 SpacePossum

Just slightly looking at that, the logic seems different, so I want to avoid bc break, I think adding more params will be ok on this class, as it will allow get non-strict usage of diff, while got functonality to set the contextLines

samsonasik avatar Dec 17 '23 16:12 samsonasik

Check, I see your point. Yet, end of day I think rector would be a better tool by offering udiff format compliant output to its users over the loose format it does now. I can see why you're creating this PR because of BC reasons, although I'm not in favor of it. Than again, it is not up to me :)

SpacePossum avatar Dec 17 '23 17:12 SpacePossum