Fixed #28184 -- Allowed passing a callable storage to FileField.

[miigotu]

Adds the ability to pass a callable as the storage param of FileField, and testing for this new ability

See ticket 28184 for details.

Signed-off-by: miigotu miigotu@gmail.com

[carltongibson]

Hi @miigotu

Thanks for the effort here. Are you still up for finishing this PR?

Some comments to be addressed:

1. The CI build still has failures, specifically isort and flake8. Can you rebase and address those please?
2. "What happens if you don't return a valid storage type?" — It's worth adding a test here. Since we can check at import time it's worth checking we get a valid return type, and probably raising ImproperlyConfigured with a helpful message if not. (Does it actually blow up immediately, or do you get a runtime error later? Is that error clear — i.e. does it instantly show what the problem is? — Etc. Better to head these off at the pass.)
3. Docs.

Let me know if you need any input.

[miigotu]

Yes, I'll fix the build first and then see about the other checks. About the type checking though, there is also no type checking in other places callables are allowed so I just did it the same way. I'll test what exactly DOES happen and see for sure.

[carltongibson]

Hi @miigotu.

Thanks for the updates. A couple of quick points...

[miigotu]

This is going to take me a bit more work/thought. These tests fail on master for me also.

[carltongibson]

Hi @miigotu — GH's review panel seems to be failing me this morning... I'll try a comment 😄

Sorry if this ends up appearing several times.

---

Hi @miigotu

Thanks for updating here. It's looking good. 👍

Does this have to wait until 2.2 now or is it allowed in 2.1.3?

It's a new feature so, it'll have to go in 2.2. Can you update accordingly?

docs/topics/files.txt:185
Feels a bit nit-picky but, is this quite right?

It's not that the storage is callable, but that the callable provides the storage... Maybe "Using a callable" or "...to specify the storage". ?

tests/file_storage/tests.py:866
Would this block be better using subTest()?

[miigotu]

@carltongibson I've addressed the changes from your last comments, and added an unforeseen way to use this in the example (mostly to make sure someone doesn't add a call method to their storage class and something unexpected happen)

This build should pass, let me know if there are any other changes that need to be made, or if my added example is too much.

[LuRsT]

@miigotu Need help with this PR?

[nikolaik]

Would have used this feature today if merged. Any help wanted?

[miigotu]

Yes. If you can get it ready I will accept PR to my repo. It will need rebased and updated.

[carltongibson]

Hi @miigotu @nikolaik.

Thanks for updating this. I pushed some edits to the docs.

Any chance we can add a test for the Storage subclass implementing __call__() case?

[miigotu]

@carltongibson I'm not sure exactly what you mean? A failure case or a success case? Is it more to exemplify how someone could have a subclass of Storage that also has a __call__ method that returns an already instantiated storage attribute?

I guess I just don't understand the goal of that one, we currently handle it the same way any other place a callable is allowed to be passed into the model manager.

[carltongibson]

Hi @miigotu. I pushed a fixup removing the callable storage example. I think it's probably clearer without it.

I'll ask @felixxm to have a look here.

[felixxm]

@miigotu Thanks 👍 I left few suggestions.

[miigotu]

@carltongibson Ahh, the callable storage example I forgot about that. So that is what you wanted a test case around, lol. I think it's probably clearer without it also, but it is one of the primary use cases where storage would adapt automagically ;)

I'll try and get some of these documentation changes in over the next few days f nobody else gets to it. I am shooting in the dark when doing this style of documentation since it's my first time.