Codecov Report

Merging #1278 (2edd69c) into master (643454c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1278   +/-   ##
=======================================
  Coverage   78.19%   78.19%           
=======================================
  Files          12       12           
  Lines        2884     2885    +1     
=======================================
+ Hits         2255     2256    +1     
  Misses        629      629           

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 643454c...2edd69c. Read the comment docs.

If you do decide to make a breaking change I wonder if requiring kwargs would be a good idea?

def __init__(
    self,
    *,
    url,
    private_token=None,
    oauth_token=None,
    job_token=None,
    ssl_verify=True,
    http_username=None,
    http_password=None,
    timeout=None,
    api_version="4",
    session=None,
    per_page=None,
    pagination=None,
    order_by=None,
    user_agent=gitlab.const.USER_AGENT,
):

And okay if people think it is a terrible idea 🙂 Just thought I would mention it.

@JohnVillalovos Not sure what you mean by requiring kwargs would be a good idea. Could you elaborate?

@JohnVillalovos Not sure what you mean by requiring kwargs would be a good idea. Could you elaborate?

Sure. So by using the * in the argument list it will require people to use keyword arguments.

Allowed:

gl = gitlab.Gitlab(url="gitlab.com")

Not allowed:

gl = gitlab.Gitlab("gitlab.com")

TypeError: __init__() takes 1 positional argument but 2 were given

It is a breaking change for sure to do that.

@JohnVillalovos I'm not so sure. I'm not really a fan of this change. This would really break everything.

@JohnVillalovos I'm not so sure. I'm not really a fan of this change. This would really break everything.

No worries. As a note you can put the * where ever you like in the arguments list. So if you wanted to require keyword arguments for everything but url you could put the * after the url argument.

def __init__(
    self,
    url,
    *,
    private_token=None,
    oauth_token=None,
    job_token=None,
    ssl_verify=True,
    http_username=None,
    http_password=None,
    timeout=None,
    api_version="4",
    session=None,
    per_page=None,
    pagination=None,
    order_by=None,
    user_agent=gitlab.const.USER_AGENT,
):

@JohnVillalovos I'm not so sure. I'm not really a fan of this change. This would really break everything.

No worries. As a note you can put the * where ever you like in the arguments list. So if you wanted to require keyword arguments for everything but url you could put the * after the url argument.

def __init__(
    self,
    url,
    *,
    private_token=None,
    oauth_token=None,
    job_token=None,
    ssl_verify=True,
    http_username=None,
    http_password=None,
    timeout=None,
    api_version="4",
    session=None,
    per_page=None,
    pagination=None,
    order_by=None,
    user_agent=gitlab.const.USER_AGENT,
):

I agree with Max I've seen a lot of use of this in tiny scripts, and there people probably want to use quick one-liners without kwargs just to instantiate Gitlab with a private token, so I'd keep this as is for now.

@JohnVillalovos while resolving conflicts I consolidated the URL redirect warnings as I saw they were in 3 places, in case you'd like to have 1 final look, but this should also be ready now.