Thanks for your work here as well @igorp-collabora!

To answer your questions:

Should calling without get_all or iterator raise deprecation warning? (mypy needs a fix python/mypy#18474)

We issue a user warning without it as it's really helped avoid frequent user questions, but I'm not sure if we should fully deprecate it/require passing it. Does it make things very difficult otherwise? /cc @JohnVillalovos

Is it get_all or all? Documentation still uses all but apparently it was replaced in favour of get_all?
**kwargs are used for all options.

all was often conflicting with API parameters, so we added get_all, yes, and silently deprecated all plus, I thought, removed any references to it in the docs. Maybe it's time we add an explicit deprecation warning to it or simply remove it in a breaking change.

Should at least some options become keyword arguments? This will improve code completion tools.

It might be a good opportunity to do this now and be more explicit. However I think we should maybe then also track removal of custom list methods (maybe we can make the list helper more generic to accommodate deviations that require custom methods now). Otherwise we'll have a lot of duplication to cover all of them.

I changed the overload to be around the iterator=. Looking at the client.py it looks like then iterator=True and page is None then the RESTObjectList is instantly returned. In all other cases the list is returned. This bypasses the get_all vs all issue.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.30%. Comparing base (beb2f24) to head (33b4a70).
Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3122      +/-   ##
==========================================
+ Coverage   97.28%   97.30%   +0.01%     
==========================================
  Files          97       97              
  Lines        5977     6012      +35     
==========================================
+ Hits         5815     5850      +35     
  Misses        162      162              

OK, this is really concise now! Focusing on the iterator arg also works great as that's where our custom iterator comes from as well. IMO, we could also add a breaking change trailer here to say that we no longer have union return but always a specific type, WDYT?

Turns out that if you want to have overloaded methods extended by some other method you need to add an extra overload with original signature:

import typing


@typing.overload
def foo(bar: typing.Literal[True] = True) -> int: ...


@typing.overload
def foo(bar: typing.Literal[False]) -> str: ...

#
# @typing.overload
# def foo(bar: bool) -> str | int: ...


def foo(bar: bool = True) -> str | int:
    return 12345 if bar else "12345"


def test() -> None:
    typing.reveal_type(foo())
    typing.reveal_type(foo(True))
    typing.reveal_type(foo(False))


@typing.overload
def baz(foz: typing.Literal[True] = True) -> int: ...


@typing.overload
def baz(foz: typing.Literal[False]) -> str: ...


def baz(foz: bool = True) -> int | str:
    return foo(foz)


def test2() -> None:
    typing.reveal_type(baz())
    typing.reveal_type(baz(True))
    typing.reveal_type(baz(False))

Without the 3rd overloaded signature on the foo the following error will be raised:

typing_overloads.py:35: error: Returning Any from function declared to return "int | str"  [no-any-return]
typing_overloads.py:35: error: No overload variant of "foo" matches argument type "bool"  [call-overload]
typing_overloads.py:35: note: Possible overload variants:
typing_overloads.py:35: note:     def foo(bar: Literal[True] = ...) -> int
typing_overloads.py:35: note:     def foo(bar: Literal[False]) -> str

I will adjust the list method overload accordingly because it is being extended in a few places. For example, SnippetManager.list_public.

Or you define the use the union of two literals to describe the extended function args type:

def baz(foz: typing.Literal[True] | typing.Literal[False] = True) -> int | str:
    return foo(foz)

This passed the type check. It looks like the bool is not the same as typing.Literal[True] | typing.Literal[False].

I think I will add the 3rd overload instead of Literal[True, False] because it will be less disruptive and more extendable for downstream users. Otherwise everyone will have to use Literal[True, False] instead of bool.

@nejch if this will be a breaking change what do you think about making the page argument not override the iterator? Right now it will fall back to a list if page is set even if iterator=True. (it also emits a warning) If the iterator will be the only argument that controls the list vs iterator then the function signature and its documentation will be cleaner.

I made the iterator argument be the only one that controls what gets returned. This makes it easier to use with the **kwargs as it type checker will not assume that page argument can be in kwargs.

See the gitlab/v4/cli.py do_list. Because we specifically call using iterator=False the type checker will know that return will be list. If page argument was still able to control the return type we would have to pass page=None or type checker will fail to match the overload as **self.args could have the page argument.

I added BREAKING CHANGES to the commit message, the exclamation mark and undrafted PR.

(also the http_list is used in a lot of managers and objects directly and they can use a return type overloads)

Thanks @igorp-collabora for looking into all the options and @JohnVillalovos for taking a look as well 🙇