request MUST be declared for serializer

[theromis]

No description provided.

[theromis]

does anybody care about pull requests?

[dessibelle]

Hi @theromis, I apologise for the monumental delay. And thanks for contributing!

Apart from being busy at work I've been putting off a few housekeeping tasks on this project such as tests and PyPI packaging for way too long, which is why I have not been able to focus on your PR until now.

I have a couple of minor questions regarding your changes, I'll add them in the code.

Also, If you like to elaborate a little on the use case for this that would be greatly appreciated as it's not 100% clear to me at this moment.

[dessibelle]

Is uri_prefix (and settings.URI_PREFIX) meant to for a path component or a complete URL, or both? I guess build_absolute_uri() would handle both.

I think I would prefer something like

from urllib.parse import urljoin
urljoin(self.uri_prefix, image.url)

or even os.path.join(self.uri_prefix, image.url) though.

[dessibelle]

Not sure how this would fare with existing usage as the current implementation attempts to use FileField.to_representation() behaviour (i.e. relative path to image) as a fallback. Up until now I've let to_representation() fail silently (which admittedly is not always the best option) but this introduces a potential AssertionError.

While I'm not too afraid of breaking changes at this stage I think it's worth some consideration. Perhaps this behaviour should only be triggered by a specific option?

[dessibelle]

A corresponding entry in the Args: list above would be nice ☺️

[theromis]

you mean you don't like settings usage here?

[dessibelle]

Sorry that's a bit unclear now that I read it myself. I just meant I'd like an the docstring above, under the Args heading.

[theromis]

but what we will gonna do with this stuff?
here uri_prefix too, I have to get rid of them.

what do you think about the rest of this PR?

[dessibelle]

Copying some of the thoughts from #5 (comment), I think you could sum up my thoughts at the moment with:

As I mentioned here HyperlinkedSorlImageField attempts to handle to_representation() transparently, in the regard that it attempts to find a sorl generated thumbnail and builds the URL from that. In case it fails, it should fallback to the default rest_framework.serializers.ImageField behaviour. This PR appears to break that by requiring request if I interpret the code correctly.

In large part I don't really understand why request is needed, as explicitly requiring (and raising if it's missing) breaks the fallback to rest_framework.serializers.ImageField.to_representation() which doesn't
rely on request.build_absolute_uri(). This is by design and something I'm not willing to let go without discussion. The current implementation does in deed try to take advantage of request if available, but can still function without it.

The same goes for url_prefix kwarg/setting which I'm not convinced is within the scope of sorl-thumbnail-serializer-field. Off the top of my head I'd say URL settings is better handled by other parts of the Django stack, and should ideally just work given that the ImageField and its corresponding serialiser is correctly setup. Please add a clear and reproducible description of your use case and solution to the PR description, and I think it'll be clearer what the intention is.

Also, tests are failing and should probably be fixed before merging :)

[theromis]

I will remove url_prefix in next commit.

You think having fqdn as an required output is wrong? I think the current logic should have something like parameter to choose full/partial URL to get, because you may expect that you will get fqdn but hard to understand why it gives you relative path, and the reason just because you forgot to pass the request how was in my case, I've spent couple hours debugging the reason.