Implement SizedSliceable protocol in src/graphql_relay/connection/arrayconnection.py

[markedwards]

Resolves #29

[Cito]

Thanks for the PR. Maybe we can already merge it, when we require the typing_extensions only for the older Python versions.

Alternatively, we could not require typing_extensions and fall back to Protocol=Sequence if Protocol does not exist in typing.

[markedwards]

I've made the requested changes, @Cito. Some notes:

• I used typing.overload to express the requirement that an array_slice arg is one of: 1) SizedSliceable or 2) Sliceable with the array_slice_length arg passed. I couldn't come up with a more elegant approach, but perhaps there is one.
• I'm not that familiar with poetry. Do I need to run poetry update here? I notice that typing-extensions is already present as a dev dependency, regardless of the python version.
• the # type: ignore on the import is annoying, but I couldn't solve it any other way

[Cito]

Looks good to me, I just had one question. Regarding poetry, you only need to run poetry install. The reason that typing-extensions are already present is that they are required by mypy.

[markedwards]

Re: poetry, I just wasn't sure if I needed to do anything other than add typing-extensions to pyproject.toml. Sounds like I'm all set there. Did you have another question?

[Cito]

I meant the question why you added a bare asterisk to the parameter list in one place (and not the others).

[markedwards]

Weird, I don't see where you originally asked the asterisk question.

So, the reason for the bare asterisk is that I need to make the array_slice_length argument mandatory for that variant, so the type-checker throws an error if it isn't passed when array_slice is a Sliceable. But I also wanted to honour the signature of the other variant because there's no reason to force keyword args for that case, and I didn't want to alter the existing runtime behaviour. I don't love the solution I ended up with, but I didn't see a better way to get the type-checker to understand my intention.

A simpler approach would be to scrap the overloads altogether and just have the runtime enforcement (i.e. a crash if a plain Sliceable is passed without array_slice_length). But that seems like a sad compromise.

Do you have a suggestion to do this more elegantly?

[Cito]

Is there a reason why you added a bare asterisk here (and not in the other overload)?

[Cito]

Sorry, forgot to finish the review, so the question was not visible.

[Cito]

Ok, right, if you remove the = None, it stops being a keyword parameter and may not follow the other keyword parameters.

[markedwards]

Yeah. Like I said, this isn't a very satisfying solution. My temptation is to rework the signatures of the connection_from_* methods to make them less intricate, but that would be a breaking change.

[Cito]

Maybe we can put the asterisk a bit further down the road, directly before the array_slice_length and add a small comment on the line with the * to explain why it's there?

Or just remove it and rely on the runtime error? I don't really like that we restrict usage with positional args in one form and not the other, just because of typing issues. But then the whole overloading and distinguishing the two scliceable types makes no sense any more.

[Cito]

Btw, if you have the energy to work on improving the typing issues, you can consider to add these to mypy.ini and make the appropriate changes (in a separate PR of course):

check_untyped_defs = True
no_implicit_optional = True
strict_optional = True

These have also been added to Graphql-core, so this would achieve more consistency.

[markedwards]

Maybe we can put the asterisk a bit further down the road, directly before the array_slice_length and add a small comment on the line with the * to explain why it's there?

I experimented with that, and it seems even more arbitrary to me. Actually, to do this correctly with overload, I suppose one would implement each possible variation, which would result in like 6 declarations. That's beyond ridiculous though. I think the situation is arguably outside the scope of overload, and this is probably not a good solution here.

Or just remove it and rely on the runtime error? I don't really like that we restrict usage with positional args in one form and not the other, just because of typing issues. But then the whole overloading and distinguishing the two scliceable types makes no sense any more.

I think the best thing to do here is ditch the Sliceable case, and just type array_slice as SizedSliceable. It's not really the job of the type signature to describe internal logical details. If it's necessary to handle the Sliceable case, there should be a separate function for this purpose which requires an array_slice_length. Besides, SizedSliceable covers the original issue I raised, which was accepting a QuerySet without casting.

[Cito]

Ok, yes, let's keep it simple. For the sake of: Beautiful is better than ugly. Simple is better than complex. Readability counts.

Unfortunately, Python type hints are not only a blessing but also a curse. Code that once looked so clean, short and simple, starts to look bloated and complicated when with all these type hints, sometimes even worse than in statically typed languages that we try to imitate.

[markedwards]

Yeah, my opinion is type hints should always improve code understandability. That's one of their major functions, IMO. They shouldn't be allowed to cross that line and make the code harder to work with.

Will update the PR.