-
Notifications
You must be signed in to change notification settings - Fork 380
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
increase test coverage report #417
Conversation
@@ -6,7 +6,7 @@ | |||
use Liip\ImagineBundle\Tests\AbstractTest; | |||
|
|||
/** | |||
* @covers Liip\ImagineBundle\Binary\Loader\StreamLoader | |||
* @covers Liip\ImagineBundle\Binary\Loader\StreamLoader<extended> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I was not aware of it
that's ok to me. You can remove pdf to image transformer too. As it could not be used without ExtendedFileSystemLoader |
mh, maybe reenable this feature, could be useful? |
@digitalkaoz it has to be done. But in a different way, with events on origin image load. Like we did for resolve PRE_RESOLVE and POST_RESOLVE |
ok removed it for now...have to dipe deeper into the current architecture to know what you mean by resolve events ;) |
so whats the goal? you request |
you have a pdf source, it has to converted to image, later filteres applied to this image and stored to cache folder. |
listener has to check that the source (by path extension or) is not image but pdf and convert it. |
and it should be stored as pdf or as an image? |
as image |
it should be done in a seperate PR or this one? |
@digitalkaoz it is completely out of this PR score. |
mh, i implemented one as you described it, havent tested it out, but looks correct to me... |
yes your right...lets do it in a separate PR... |
what about merging this one? |
@digitalkaoz sure, great work! |
by writing some new tests and applying missing covers annotations.
removed obsolete
Binary/Loader/ExtendedFileSystemLoader.php
since it could not work (wrong constructor and things) but i think its ok to remove