-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Add signature #179
Add signature #179
Conversation
If I see it correctly from the diff, you've created an optional |
Amusingly I didn't even think of adding it to the Builder pattern until you mentioned it. In part because I think it's so closely tied to the model you pass in. You could make an argument that the builder arguments apply generally to a style of load for any model, but the signature typically would apply only to a specific model (url etc). I'm not sure that's a good enough reason to special case it though? |
Try to look from the user point of view... and use it in a sentence: Second I personally like to not repeat code and the way I used Picasso and then Glide was to create my own factories pre-attaching the error Drawable, transformation and other stuff and returning a builder to call Third, when you say it's tied to the model it feels like you're only thinking about the opposite of using Please note that I like to trigger thoughts in other people and defend my initial view, though usually it's really easy to find arguments for both sides. You most likely see now where my original question came from and have enough to make an informed decision. |
Thanks for the feedback! I definitely appreciate having another point of view. I've changed this so that Hopefully this is more in line with what you were thinking? I agree it's strange to pass in the signature along with the model. |
Thanks for taking a look, I always appreciate the feedback! I pushed a few more commits to fix the issues we've found so far. If no one objects I'll aim to rebase this in tomorrow. |
Fixes bumptech#176.
Allows users to easily mix in additional data into cache keys.
This would fix #178, and would be a step in the right direction for #174 and #172