search

singingwolfboy / flask-dance

Doing the OAuth dance with style using Flask, requests, and oauthlib.
https://pypi.python.org/pypi/Flask-Dance/
MIT License
1.01k stars 158 forks source link

use keyword-only arguments #360

Closed singingwolfboy closed 3 years ago

singingwolfboy commented 3 years ago

See https://www.python.org/dev/peps/pep-3102/. I intended to use this feature when I first created this library, but Python 2.7 compatibility prevented me from doing so, and I forgot about it until recently. Since this has the potential of causing backwards-incompatibility, we will need to bump the major version number.

@daenney, do you have any thoughts about this change?

codecov[bot] commented 3 years ago

Codecov Report

Merging #360 (e33e9a9) into main (db3c2fc) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #360   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files          34       34           
  Lines        1045     1045           
=======================================
  Hits          979      979           
  Misses         66       66
Impacted Files Coverage Δ
flask_dance/consumer/base.py 89.87% <ø> (ø)
flask_dance/consumer/oauth1.py 68.60% <ø> (ø)
flask_dance/consumer/oauth2.py 92.23% <ø> (ø)
flask_dance/contrib/atlassian.py 100.00% <ø> (ø)
flask_dance/contrib/authentiq.py 100.00% <ø> (ø)
flask_dance/contrib/azure.py 100.00% <ø> (ø)
flask_dance/contrib/digitalocean.py 100.00% <ø> (ø)
flask_dance/contrib/discord.py 100.00% <ø> (ø)
flask_dance/contrib/dropbox.py 100.00% <ø> (ø)
flask_dance/contrib/facebook.py 100.00% <ø> (ø)
... and 16 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 db3c2fc...e33e9a9. Read the comment docs.

daenney commented 3 years ago

I like this. I've always found the behaviour where positional args can fill in keyword args confusing. It's almost always an error, and I like the self-documenting nature of keyword args.

One question though, in your changeset you're keeping client_id and client_secret as old-style arguments, kwarg but could be filled by positional. Why is that?

singingwolfboy commented 3 years ago

One question though, in your changeset you're keeping client_id and client_secret as old-style arguments, kwarg but could be filled by positional. Why is that?

I did that because client_id and client_secret are necessary for Flask-Dance to work, and so they are almost like positional, required arguments. I intentionally kept them as the first two arguments in every pre-set configuration, for that reason. The only time that it's OK to not specify those arguments is when you are loading those values in the flask.config object instead, which is why they are not actually positional, required arguments. You'll also notice that the automated tests use this pattern a fair amount, specifying the client_id and client_secret as positional arguments. I want that to be a supported pattern, since it seems fairly clear to me.

Does that make sense, and do you agree?

daenney commented 3 years ago

Yup, that makes sense!