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

Add ability to set contextLines in __construct() on UnifiedDiffOutputBuilder #122

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

samsonasik
Copy link

No description provided.

@sebastianbergmann sebastianbergmann added the enhancement Indicates new feature requests label Dec 17, 2023
Copy link

codecov bot commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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           

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

@samsonasik
Copy link
Author

I fixed psalm notice by add @psalm-param positive-int $contextLines f3b2657

@SpacePossum
Copy link
Contributor

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

@samsonasik
Copy link
Author

@SpacePossum
Copy link
Contributor

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.

@samsonasik
Copy link
Author

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

@SpacePossum
Copy link
Contributor

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants