Open lcdunne opened 11 months ago
Hi, thanks. It would be great if you can add a test case for this method.
Sure I'll give that a go. Is there a development build that installs all dependencies needed for testing? And any other information on how you are running the tests?
The whole testing process is automated with tox. Just run tox, and this will create environments for every python version supported, and a few dependency variants, and this will run pytest on those environments.
Can I give a suggestion to change the method name to unregister
instead so the naming convention is kept in the same family so to say. Would also suggest the code to be more forgiven for odd behaviors (unlikely with this code but nonetheless a great failsafe).
Code suggestion:
def unregister(self, name):
err = None
if name not in self._registry:
err = f"Client {name} not found in registry."
else:
del self._registry[name]
if name not in self._clients:
err = f"Client {name} not found in clients."
else:
del self._clients[name]
if err:
raise KeyError(err)
The method create_client
first checks if self._clients
contains name
and if the unregister
method do not finish deleting everything, you could end up with a half-deleted state where create_client
returns a client that the registry does not have.
Can I give a suggestion to change the method name to unregister instead so the naming convention is kept in the same family so to say.
The register naming is not ideal anyways as it could be confused with registration like defined in OAuth 2.0 Dynamic Client Registration Protocol.
In the long run I think keeping remove_client
but move register
to add_client
would be a better idea.
Also, can you link this PR to that issue ( #583 ), like this?
What kind of change does this PR introduce? (check at least one)
Feature to support removing a client from the
oauth
instance (see this issue). This works with the Flask client but I am not familiar with the other frameworks enough to set them up to check. As I mentioned in the issue thread it really looks like those other clients inherit from BaseOAuth but please let me know if I'm overlooking something. I'll try to add what is needed.