Skip to content

Conversation

SpacePossum
Copy link
Contributor

Sadly, after the release of v2 not all are happy with it.
I'm sorry and hereby apologize to everyone who is effected by the changes.

The intent for v2 was to provide an extendable diff library together with some issue resolving which required BC breaks.
However some changes were not up to par and need work.

This PR is the start of resolving the reported issue.
First of I want to create a nice and clean test suite for the package such that new changes can be reviewed more easy.

The changes are:

  • Exception - Move to own namespace. (I'm still puzzled why this worked with PSR4)
  • UnitTest - Code grooming.
    • move test to own namespace
    • break up DifferTest to dedicated test per class

To be clear, besides the namespace change of the Exceptions, there are no functional changes to the package in this PR (only to its tests).

Ping @keradus if you've time please do a sanity check :)

@sebastianbergmann
Copy link
Owner

  • I do not use PSR-4 (or similar) for my components
  • Please use classmap-based autoloading
  • Please do not put exceptions into a separate namespace

@codecov-io
Copy link

codecov-io commented Dec 14, 2017

Codecov Report

Merging #73 into master will decrease coverage by 37.31%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master      #73       +/-   ##
=============================================
- Coverage     99.74%   62.43%   -37.32%     
  Complexity      150      150               
=============================================
  Files            10       10               
  Lines           397      386       -11     
=============================================
- Hits            396      241      -155     
- Misses            1      145      +144
Impacted Files Coverage Δ Complexity Δ
src/Output/DiffOnlyOutputBuilder.php 0% <ø> (-100%) 10 <0> (ø)
src/Differ.php 73.94% <100%> (-25.22%) 52 <0> (ø)
src/Output/AbstractChunkOutputBuilder.php 0% <0%> (-100%) 8% <0%> (ø)
src/Output/UnifiedDiffOutputBuilder.php 4.05% <0%> (-95.95%) 26% <0%> (ø)
src/Parser.php 100% <0%> (ø) 19% <0%> (ø) ⬇️
src/Chunk.php 100% <0%> (ø) 7% <0%> (ø) ⬇️
src/Line.php 100% <0%> (ø) 3% <0%> (ø) ⬇️
src/Diff.php 100% <0%> (ø) 5% <0%> (ø) ⬇️

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 52a3981...e125c6b. Read the comment docs.

@SpacePossum
Copy link
Contributor Author

SpacePossum commented Dec 14, 2017

I do not use PSR-4 (or similar) for my components
Please use classmap-based autoloading
Please do not put exceptions into a separate namespace

all done (I think)

not sure what is going on with the coverage report, no tests have been removed

@SpacePossum SpacePossum changed the title Output format issues Clean up the test suit Dec 14, 2017
@SpacePossum SpacePossum mentioned this pull request Dec 14, 2017
@sebastianbergmann
Copy link
Owner

Please keep test case classes in the same namespace as the class they're testing.

@SpacePossum
Copy link
Contributor Author

Please keep test case classes in the same namespace as the class they're testing.

updated

@sebastianbergmann
Copy link
Owner

Thanks for your quick responses.

Is this ready to be merged?

@SpacePossum
Copy link
Contributor Author

Squashed to get a clean commit for the changes, if the tests are green it should be good.
However I'm not sure why the coverage report comes back so poorly.

@sebastianbergmann sebastianbergmann merged commit c6f30b2 into sebastianbergmann:master Dec 14, 2017
@sebastianbergmann
Copy link
Owner

Don't worry about the @covers annotations (the reason for low coverage right now). I'll take care of that.

@SpacePossum
Copy link
Contributor Author

👍 thanks :)

@SpacePossum SpacePossum deleted the master_fixes branch December 14, 2017 10:51
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