dotblue / nette-webimages

On-the-fly generated web images for your Nette app
Other
26 stars 18 forks source link

Refactor #9

Open lookyman opened 10 years ago

lookyman commented 10 years ago

Ok, this PR might be a little over the top, but I started playing with it and ended up with this. It's still not finished, I need to add more tests for Extension and especially Route, do some more prettying, clean up, and stuff like that. But I want to hear your opinions.

Summary of changes

vojtech-dobes commented 10 years ago

Hi, thanks for all great work! Please let me know when you are satisfied with it, then I will walk through it and comment (some things I like, some don't).

lookyman commented 10 years ago

Well, basically I think it's ready, except for tests. Ofc those could show something is broken, but the general shape should not change much even if they do..

lookyman commented 10 years ago

Ok now I think I'm finished. There's always more that could be done, but that can be left to future PRs. Let me know after you've read through it..

f3l1x commented 10 years ago

I like

I don't think is needed

lookyman commented 10 years ago

I can get rid of route prepending, or possibly add a configuration option to prepend instead of default append. Basically the only reason I added it in the first place was to work around a problem I had in one of my projects, and this was the easiest way (modify the thing I am trying to add instead of touching the stuff I have already written).

I'll wait for @vojtech-dobes, I'm sure he'll have a lot to add.

Koricz commented 9 years ago

I really looking forward when this PR will be merged, but I think there still should be an option passing all parameters from template link to Generator and Provider(s)...

https://github.com/lookyman/nette-webimages/blob/fc9e83e54a7b9651bcf76b18f158c0d6838e8e6e/src/Application/Route.php#L57