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

increase test coverage report #417

Merged
merged 1 commit into from
Apr 27, 2014
Merged

increase test coverage report #417

merged 1 commit into from
Apr 27, 2014

Conversation

digitalkaoz
Copy link
Contributor

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

@@ -6,7 +6,7 @@
use Liip\ImagineBundle\Tests\AbstractTest;

/**
* @covers Liip\ImagineBundle\Binary\Loader\StreamLoader
* @covers Liip\ImagineBundle\Binary\Loader\StreamLoader<extended>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

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

@makasim
Copy link
Collaborator

makasim commented Apr 25, 2014

removed obsolete Binary/Loader/ExtendedFileSystemLoader.php since it could not work (wrong constructor and things) but i think its ok to remove

that's ok to me. You can remove pdf to image transformer too. As it could not be used without ExtendedFileSystemLoader

@digitalkaoz
Copy link
Contributor Author

mh, maybe reenable this feature, could be useful?

@makasim
Copy link
Collaborator

makasim commented Apr 25, 2014

@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

@digitalkaoz
Copy link
Contributor Author

ok removed it for now...have to dipe deeper into the current architecture to know what you mean by resolve events ;)

@digitalkaoz
Copy link
Contributor Author

so whats the goal? you request foo.pngwith filter pdf and want to have foo.pdf resolved?

@makasim
Copy link
Collaborator

makasim commented Apr 25, 2014

you have a pdf source, it has to converted to image, later filteres applied to this image and stored to cache folder.

@makasim
Copy link
Collaborator

makasim commented Apr 25, 2014

listener has to check that the source (by path extension or) is not image but pdf and convert it.

@digitalkaoz
Copy link
Contributor Author

and it should be stored as pdf or as an image?

@makasim
Copy link
Collaborator

makasim commented Apr 25, 2014

as image

@digitalkaoz
Copy link
Contributor Author

it should be done in a seperate PR or this one?

@makasim
Copy link
Collaborator

makasim commented Apr 25, 2014

@digitalkaoz it is completely out of this PR score.

@digitalkaoz
Copy link
Contributor Author

mh, i implemented one as you described it, havent tested it out, but looks correct to me...

@digitalkaoz
Copy link
Contributor Author

yes your right...lets do it in a separate PR...

@digitalkaoz
Copy link
Contributor Author

what about merging this one?

makasim added a commit that referenced this pull request Apr 27, 2014
increase test coverage report
@makasim makasim merged commit 0b20545 into liip:master Apr 27, 2014
@makasim
Copy link
Collaborator

makasim commented Apr 27, 2014

@digitalkaoz sure, great work!

ama3ing pushed a commit to ama3ing/LiipImagineBundle that referenced this pull request Apr 28, 2014
makasim added a commit that referenced this pull request Apr 28, 2014
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.

2 participants