search

lehins / django-smartfields

Django Model Fields that are smart.
MIT License
98 stars 13 forks source link

#29 Integrated django-storages #30

Closed null-none closed 3 years ago

null-none commented 3 years ago

29 Integrated django-storages

coveralls commented 3 years ago

Coverage Status

Coverage decreased (-0.05%) to 90.853% when pulling e7a0b4c00558dea450bc30e6874c0ae2158c1e37 on null-none:master into a35726643aa3bc65040bf2fc2ece7e2c3c8538a2 on lehins:master.

lehins commented 3 years ago

To be honest with you, I am not that fond of the proposed solution, it is great however that you identified the place where the fix is needed. I don't like this solution for a very simple reason, it will silently break behavior for every user that specifies custom class for default storage. For example if user's storage is some derivative of django.core.files.storage.FileSystemStorage that requires local full path, all of a sudden their code will break. I think a better solution would be an opt-in mixin:

class CloudExternalFileProcessorMixin(object):
    def get_input_path(self, in_file):
        return in_file.instance.file.url

Then the user can use such mixing to opt-in into a different behavior with:

class CloudFFMEGPRocessor(CloudExternalFileProcessorMixin, FFMPEGProcessor):
  pass

Another solution that I think might be even better is to give ability to override this method at construction time. Could you try out this PR #31 and see if it solves the problem that you are experiencing?

null-none commented 3 years ago

@lehins My PR isn't perfect, I agree. I just showed how you can solve the problem. I check PR the mixin version today and write to you.