search

openx / OX3-Python-API-Client

Helper class for accessing the OX3 API - Requires https://github.com/simplegeo/python-oauth2
Other
8 stars 17 forks source link

add required param 'realm' for client_from_file #11

Closed proles closed 5 years ago

proles commented 9 years ago

Hello,

Without the inclusion of realm, performing an authentication that is based on the .ox3rc file would fail due to the Client not being initialized with the right amount of params.

This commit addresses this issue and allows for the successful authentication and logon of a user with the client_from_file method.

Thanks.

Andrés

TimWhalen commented 9 years ago

I talked with Tony Edwards (the original author of this package) and we think realm should now go in the optional_params list for client_from_file (as it's no longer a required param for the client init method.) This would fix the existing unit tests which were broken when realm was removed. If you agree would you make that change and resubmit your pr?

tnydwrds commented 9 years ago

Thanks for the PR Andrés.

On second thought adding realm to the optional_params list wouldn't work since realm is a positional argument for the Client constructor. In hindsight it would have been best to use keyword arguments instead of using positional arguments as cheap validation of required parameters. This way we wouldn't have had to be concerned with maintaining backward compatibility when realm was made optional.

Anyway, since realm is now optional for authentication can you update your PR to allow the realm to be optional in the .ox3rc file? Maybe passing None in the constructor if realm is not in the file?

That way we could at least hide the superfluous realm from folks using the client_from_file helper.

Regards, Tony

proles commented 9 years ago

Hi Tony and Tim,

Thank you for your feedback. I have updated the inclusion of realm to be an optional_param for the client_from_file function, and in doing so, all unit test are now successful.

I hope you find the changes to be satisfactory, and while I considered updating the tests themselves to reflect the fact that realm was no longer a required item, I figured keeping those around within the context of the updated documentation would be acceptable.

Let me know if you have any additional suggestions.

Thanks,

Andrés