Bacon / BaconQrCode

QR Code Generator for PHP
BSD 2-Clause "Simplified" License
1.83k stars 208 forks source link

Eye rotation doesn't work with inherited colors #106

Closed mustanggb closed 6 months ago

mustanggb commented 2 years ago

Fixes #105.

codecov-commenter commented 2 years ago

Codecov Report

Merging #106 (5549ad5) into master (5c2d190) will increase coverage by 2.07%. The diff coverage is 80.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #106      +/-   ##
============================================
+ Coverage     63.42%   65.49%   +2.07%     
- Complexity      928      939      +11     
============================================
  Files            47       48       +1     
  Lines          2783     2855      +72     
============================================
+ Hits           1765     1870     +105     
+ Misses         1018      985      -33     
Impacted Files Coverage Δ
src/Renderer/Path/Curve.php 0.00% <0.00%> (ø)
src/Renderer/ImageRenderer.php 91.52% <50.00%> (-3.03%) :arrow_down:
src/Renderer/Eye/GridEye.php 100.00% <100.00%> (ø)
src/Renderer/Path/Close.php 100.00% <100.00%> (ø)
src/Renderer/Path/EllipticArc.php 34.67% <100.00%> (+34.67%) :arrow_up:
src/Renderer/Path/Line.php 100.00% <100.00%> (ø)
src/Renderer/Path/Move.php 100.00% <100.00%> (ø)
src/Renderer/Path/Path.php 89.18% <100.00%> (+14.18%) :arrow_up:
src/Renderer/RendererStyle/Fill.php 82.92% <0.00%> (+4.87%) :arrow_up:
src/Renderer/Image/ImagickImageBackEnd.php 69.03% <0.00%> (+5.80%) :arrow_up:
... and 2 more

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 5c2d190...5549ad5. Read the comment docs.

mustanggb commented 2 years ago

If you want a test is it okay to add my non-symmetrical eye interface to the codebase?

mustanggb commented 2 years ago

No idea why the test is failing, is there anyway to see the file generated by the CI?

mustanggb commented 2 years ago

Okay, figured it out.

The code was fine and the files were identical to the human eye.

Just slight differences probably due to imagick version differences between my system and the test platform.

So I extracted the files from a workflow artifact and now the tests pass.

DASPRiD commented 2 years ago

An alternative would be to test with the SVG renderer, since that output is predictable.

mustanggb commented 2 years ago

Yup, definitely a safer option, but perhaps better as a follow-up to update all the integration tests.

mustanggb commented 2 years ago

@DASPRiD Any feedback, or things you'd like addressed?

DASPRiD commented 2 years ago

Generally this looks good to me. I'm not sure about the GridEye naming, that's not very descriptive for what it actually is. I cannot think of a better term from the top of my head right now though.

DASPRiD commented 2 years ago

How about you rename it to PointyEye, I think that might be more descriptive. Apart from that, the PR looks good to me.

DASPRiD commented 6 months ago

@mustanggb I haven't heard back from you ever. I'm currently preparing a new major release. If I don't hear back from you in the next few days, I'm going to close this PR.

mustanggb commented 6 months ago

Heard back about what?

I was just waiting for it to be merged.

DASPRiD commented 6 months ago

@mustanggb See my comments from March 14 and 27, 2022 :)

DASPRiD commented 6 months ago

Also, please rebase against the new main branch.

mustanggb commented 5 months ago

Just a rename then, I don't mind it.

Rename and rebase at #174.