Skip to content
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

Refactor CONSUME_MULTIPLE_SEGMENTS in BaseConverter #2206

Open
davetapley opened this issue Jan 29, 2024 · 1 comment
Open

Refactor CONSUME_MULTIPLE_SEGMENTS in BaseConverter #2206

davetapley opened this issue Jan 29, 2024 · 1 comment

Comments

@davetapley
Copy link

davetapley commented Jan 29, 2024

I was going to add types to

class BaseConverter(metaclass=abc.ABCMeta):

but as the docs give the type of value ⬇️ depend on CONSUME_MULTIPLE_SEGMENTS.

def convert(self, value):
"""Convert a URI template field value to another format or type.
Args:
value (str or List[str]): Original string to convert.
If ``CONSUME_MULTIPLE_SEGMENTS=True`` this value is a
list of strings containing the path segments matched by
the converter.

This is a bit awkward because CONSUME_MULTIPLE_SEGMENTS can only be known at run time, but it's so tightly coupled to the behavior of the class it feels like it should be static.

Can I propose having two classes instead, e.g. BaseConverter and BaseConverterMultipleSegment, with value as str and list[str] respectively?

Then we could probably get rid of ⬇️ and just do an isinstance check.

def _consumes_multiple_segments(converter):
return getattr(converter, 'CONSUME_MULTIPLE_SEGMENTS', False)

@CaselIT
Copy link
Member

CaselIT commented Jan 30, 2024

Hi,

The main reason for the current design is that converters for historical reason do not need to be subclasses of BaseConverter, but ducktyping is used.

I'm not opposed to changing the requirement to have them being a sublcass of BaseConverter and BaseConverterMultipleSegment but it would likely be a v5 since it would be nicer if v4 would just raise deprecation warnings for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants