search

falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

Refactor CONSUME_MULTIPLE_SEGMENTS in BaseConverter #2206

Open davetapley opened 8 months ago

davetapley commented 8 months ago

I was going to add types to https://github.com/falconry/falcon/blob/4910dd73ecd1b9c8cf6cae045b26ad432fa56128/falcon/routing/converters.py#L34

but as the docs give the type of value ⬇️ depend on CONSUME_MULTIPLE_SEGMENTS. https://github.com/falconry/falcon/blob/4910dd73ecd1b9c8cf6cae045b26ad432fa56128/falcon/routing/converters.py#L46-L53

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. https://github.com/falconry/falcon/blob/4910dd73ecd1b9c8cf6cae045b26ad432fa56128/falcon/routing/converters.py#L61-L62

CaselIT commented 8 months ago

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.