FusionAuth / fusionauth-python-client

FusionAuth Python Client
https://fusionauth.io
Apache License 2.0
24 stars 12 forks source link

Use Python optional parameters #5

Closed andystanley closed 4 years ago

andystanley commented 4 years ago

Thanks for all your work on this library!

Small suggestion, but I believe making use of Python's optional parameters would greatly improve the usability of this library.

For example, the exchange_user_credentials_for_access_token method has 4 optional parameters, but they still need to be specified.

If I just wanted to specify the scope it would look like:

client.exchange_user_credentials_for_access_token(
  username='username', 
  password='password', 
  client_id=None, 
  client_secret=None, 
  scope='scope', 
  user_code=None)

However, I would prefer to do:

client.exchange_user_credentials_for_access_token(
  username='username',  
  password='password', 
  scope='scope')

This can be accomplished by changing the method signature to specify None for the optional parameters:

def exchange_user_credentials_for_access_token(self, 
  username, 
  password, 
  client_id=None, 
  client_secret=None, 
  scope=None, 
  user_code=None):

Let me know what you think. I can definitely put a PR up for this change!

robotdan commented 4 years ago

Thanks for the suggestion @andystanley we're always looking for ways to better align with the idioms of each language we support.

I'm not a Python expert, but we have a few on our team. @trex and @matthew-altman any opinion here?

A PR would be great. This library is built using a template builder.

Currently the optional parameters are only designated in the comment, we may have to come up with another attribute in our API DSL to denote an optional parameter so the client template can optionally take advantage.

Here are the templates and moving parts if you want to take a look.

https://github.com/FusionAuth/fusionauth-client-builder https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/client/python.client.ftl https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/api/exchangeUserCredentialsForAccessToken.json

andystanley commented 4 years ago

Cool! I wasn't aware this library was built with a template builder, but I'll take a look and see if it's something I could put a PR up for.

andystanley commented 4 years ago

I've taken a stab at this and opened a PR for this change in the https://github.com/FusionAuth/fusionauth-client-builder repo.

robotdan commented 4 years ago

Great, thanks for the PR @andystanley we'll take a look.

matthew-altman commented 4 years ago

Related PR was accepted.