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

POC add line numbers #64

Merged
merged 2 commits into from
Jun 25, 2017
Merged

POC add line numbers #64

merged 2 commits into from
Jun 25, 2017

Conversation

SpacePossum
Copy link
Contributor

@SpacePossum SpacePossum commented Jun 22, 2017

This PR introduces the line numbers to the UnifiedDiffOutputBuilder (finally ;) )

However there a few things to consider;

  • the warning inserted about line break differences cause the output of the output builder to be not compat with the patch tool
  • the output builder does not add the \No newline at end of file to the output when a string has not a line break as last character, making the output not compat with the patch tool
  • with above exceptions the output follows the unified diff format, like patch -u and can be consumed like diff -u
  • the output is now minimal around the chunks, we might pick a better default;
    for example diff picks 3 lines above and below a changed line ( -u, -U NUM, --unified[=NUM] output NUM (default 3) lines of unified context)
  • above might be a valuable option to be passed through the constructor to allow more tweaking

I left the incompats because;

  • removing a feature that people might like (the warning) is not nice
  • the \No newline at end of file is only of value when diff'ing files, and not arbitrary strings as typically done by PHPUnit

I plan on writing a full compat version of the output builder so we can offer it 3rd party.

Let me know what you all think :)

ping @keradus @julienfalque please have a look if you've time

PS.
The last commit contains a test that is prop. best not merged to the repo, but is very handy when debugging.

@codecov-io
Copy link

Codecov Report

Merging #64 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #64      +/-   ##
============================================
+ Coverage     99.71%   99.73%   +0.02%     
- Complexity      138      146       +8     
============================================
  Files            10       10              
  Lines           347      377      +30     
============================================
+ Hits            346      376      +30     
  Misses            1        1
Impacted Files Coverage Δ Complexity Δ
src/Output/UnifiedDiffOutputBuilder.php 100% <100%> (ø) 26 <15> (+8) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf53e7...0ad04de. Read the comment docs.

@keradus
Copy link
Contributor

keradus commented Jun 25, 2017

The last commit contains a test that is prop. best not merged to the repo, but is very handy when debugging.

Actually, I would keep it

@sebastianbergmann
Copy link
Owner

The test in question interacts with the filesystem and assumes that patch is on the path. I think there is value in this as an integration test but it will of course not work on Windows, for instance.

@keradus
Copy link
Contributor

keradus commented Jun 25, 2017

of course it would not. that's why there is require OS
if the diff is suppose to be compatible with unified diff format, there is no better way to check it.

@sebastianbergmann
Copy link
Owner

Sure; but even on Linux patch might not be available. I want this test, though :)

@sebastianbergmann sebastianbergmann merged commit 2994073 into sebastianbergmann:master Jun 25, 2017
@keradus
Copy link
Contributor

keradus commented Jun 25, 2017

@SpacePossum , is there anything else in big epic of introducing line numbers ?

@SpacePossum SpacePossum deleted the master_add_line_numbers branch June 25, 2017 12:10
@SpacePossum
Copy link
Contributor Author

SpacePossum commented Jun 25, 2017

@keradus I've nothing major planned left for this.
At the moment I can see some refinements that can be made which I've listed in the description of the PR.
Like to hear what others think about the current state of the package and what features people are missing.
In terms of health, the repo has a near 100% test coverage, CS looks fair, performance and complexity looks good to me based on what the code does.

(thanks for reviewing/merging all btw. :) I'll clean up the test a bit more in the near future )

@sebastianbergmann
Copy link
Owner

@SpacePossum Are we ready for the 2.0.0 tag?

@keradus
Copy link
Contributor

keradus commented Jun 25, 2017

And are any of those refinements BC breakers ?

@SpacePossum
Copy link
Contributor Author

SpacePossum commented Jun 26, 2017

since the API has been closed down as it is now (final on the classes) I think all ideas for refinements/additions can be done later without BC breakers.

@sebastianbergmann
Copy link
Owner

@SpacePossum May I tag 2.0.0? Then the next step is to update PHPUnit etc. to the new API.

@SpacePossum
Copy link
Contributor Author

@sebastianbergmann lets do it :)
The changes on PHPUnit should be relative easy to do, lemme know if I can help out

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.

4 participants