Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix imports on the management/__init__.py file #235

Merged
merged 4 commits into from
Aug 28, 2020

Conversation

matthewarmand
Copy link
Contributor

@matthewarmand matthewarmand commented Aug 20, 2020

Changes

Very simple PR, more of a client developer quality-of-life change than anything, feel free to disregard if not important or wanted.

Currently, importing Hooks from the management module like so is not possible:

from auth0.v3.management import Hooks

This is because in management's __init__.py, Hooks is not imported. It is still possible to import Hooks with the following code:

from auth0.v3.management.hooks import Hooks

But when so much else is available at the management module level its kind of a pain.

Testing

This is a minute change without any implications on functionality, as mentioned above just a quality of life change. Automated testing seems inappropriate, and building the package and verifying that from auth0.v3.management import Hooks is a valid line of code should be sufficient (if even that's necessary, since it follows the pattern of many other modules within management.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@matthewarmand matthewarmand requested a review from a team August 20, 2020 14:14
@lbalmaceda
Copy link
Contributor

It looks like the use of the __init__.py file in directories only works for python 2.x. And according to this SO question python 3 no longer needs this for that import to work. Can you confirm this @matthewarmand, please?

Anyway, yesterday I added a new entity to the client, and I noticed if not directly importing that file (as in your second snippet), you could use the management/auth0.py class which creates every entity for you. I'll accept your changes and will also add the one I added yesterday. But would like to hear from you what version of python you're targetting.

@lbalmaceda lbalmaceda changed the title Import hooks in management init so that importing Hooks from management is possible Fix imports on the management/__init__.py file Aug 28, 2020
@lbalmaceda lbalmaceda added the tiny Tiny review label Aug 28, 2020
@lbalmaceda lbalmaceda added this to the v3-Next milestone Aug 28, 2020
@lbalmaceda lbalmaceda merged commit b580ce0 into auth0:master Aug 28, 2020
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.13.0 Aug 28, 2020
@matthewarmand
Copy link
Contributor Author

Hi @lbalmaceda , I appreciate the feedback and merge.

according to this SO question python 3 no longer needs this for that import to work.

If you look at the highest rated answer below the "Accepted" answer on that SO question (handy link here) you'll see an important caveat that Python 3.3+ supports implicit module imports ONLY if the __init__.py is empty.

That said I'm also a bit unclear on the exact implications of that. I used docker to build/install the library and test imports in a shell and ran four scenarios using Python 3.8.3:

  1. Run the version in my branch (with Hooks import included in __init__.py)
  2. Remove Hooks import from __init__.py (state previous to my fix)
  3. Remove all imports from __init__.py, attempting to leverage Python's implicit module definition as mentioned in the SO answers
  4. Remove __init__.py entirely, attempting to leverage Python's implicit module definition (with empty init optional) as mentioned in the SO answers

Of all of these, scenario 1 was the only one in which I was able to successfully import the Hooks module from within a Python 3.8.3 shell.

Theoretically, per those SO answers it seems one should be able to allow the module to be implicitly imported, but in my limited testing I can't seem to find the correct set of circumstances to make that happen.

I hope that helps, let me know if there's any other context I can offer or questions I can answer

@matthewarmand matthewarmand deleted the import-hooks-fix branch August 28, 2020 18:17
@lbalmaceda
Copy link
Contributor

Thanks for trying that and sharing your scenarios. I guess we will have to keep adding those in the file then. I'll add an internal note so the next person that maintains the SDK remembers to add that import line in the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants