Explicit global _keyring_backend and better dynamic import

Alan Franzoni avatarAlan Franzoni created an issue

Since they matched a closely related functionality in the very same file, those two features were actually merged into a single one.

1) the way _keyring_backend was used in core.py relied on the way the _keyring_backend name was set during import. This could work but could lead to very strange issues sometimes.

The patch provides an explicit global keyword which allows performing the very same task in a cleaner way. NOTE: _keyring_backend was actually meant to be a global even before this patch, but it was used inconsistenly. Looking at all the functions employing _keyring_backend, they will simply crash with a NameError if _keyring_backend is unset, but how is it set is unclear.

A better approach would be use a registry and not to rely on global, I'll try that in a further patch.

2) Don't reinvent the wheel and use a tested function to dynamically import Keyring backend classes. This patch employs a function from the Twisted project with small modifications.

Comments (8)

  1. Alan Franzoni

    Eric: you're perfectly right, thank you for noticing.

    While they did no harm they served absolutely no purpose.

    The real "fix" is the _keyring_backend being explicitly set at the beginning of the module, for the sake of clarity and to prevent strange runtime-order errors.

    Here it is a corrected version of such patch.

    EDIT I got the wrong file attached and I can't remove it. Ignore the v2 patch and take a look at the v3 patch.

  2. Jason R. Coombs

    Hi Alan. Thanks for the patches and sorry this wasn't addressed sooner.

    After reviewing this patch in detail, I think I'm going to reject it. It suggests some good ideas, but it has a few issues.

    • First, the patch is stale and conflicts with some refactoring already done that's very similar (replacing the module resolution with a function). This, of course, was not your fault but ours.
    • The patch attempts to do too much (especially for a diff). It would be better to make several changes in successive commits in a pull request.
    • While it borrows from another project for resolving the module, I feel the twisted implementation is also deprecated at this point. I'd prefer to use 'importlib' now.
    • The patch removes checks for around error handling (the try/except block for ConfigParser errors), but there's no comment indicating why this change is relevant.
    • The patch removes support for importing modules relative to the config path name.
    • Finally, while the patch description indicates that functions will 'crash' with a NameError, if I understand correctly, the will continue to crash, just with an AttributeError.

    I agree there are improvements to be made here, so I don't want to be too discouraging. I do encourage you to continue to contribute. As you do, please try to file tickets with a specific issue, describe that issue (how it's encountered and what you expect to happen differently), and a specific patch or pull request that addresses that issue. Including tests that demonstrate the undesirable behavior and are fixed by the patch are also encouraged.

  3. Alan Franzoni

    Jason, I appreciate your interest in fixing keyring library issues. I'm sure it's not your fault that it took so long - different people were handling issues at the time.

    Time has passed and the source code of keyring changed, so some of my concerns are not applicable anymore right now.

    Nevertheless, I think that the time I took writing the patch and the time I waited for a response would deserve a bit more thorough review of my patch. You didn't need to apply it - it would make little sense on today's codebase, I suppose - but an answer like "I'm sorry we took so much time. Your patch is now outdated, and we won't accept it" would have been much, much better.

    1) the patch is stale -> of course I can do nothing about that

    2) the patch attempts to doo to much -> maybe a really small bit. Maybe it would have been good to split the _keyring_backend modification and the better import. But at the time the project was quite messy and the small modification of adding a _keyring_backend (which was a good idea, now a similar result is achieved in init_backend(), I see) at the beginning in a single commit would do little harm. The other part is quite a lot of code, but from a semantic POV it's a single change.

    3) Sure, it may be deprecated. By the way python 2.7 with importlib was just a few months old when I released this patch, and at the time I thought that it was a bit limiting to prevent python < 2.7 to work with keyring, or to require an external library for a single functionality. In hindsight, today importlib would be a good choice.

    4) There's no comment but the code is clear. The original code made little sense. There's no way ConfigParser could raise the error - the original code checks for such an option in a safe way ( config.get ... ) then raises NoOptionError and catches it later. Since the patch proposal makes the code much shorter, such internal try/catching makes no sense and it's just a matter of if/else. Behaviour is just the same.

    5) That's true; I should probably have deleted that as well, since my idea was to just let the keyring be loaded from its FQDN, so it should be in sys.path elsewere. I still think that's a better way to go (manual fiddling with load path is a bad idea, IMHO)

    6) "NoneType" has no attribute "something" tells much more than a stray NameError. I couldn't fix everything with a single patch. A more complex proposal I submitted years ago to the keyring ML (no answer, of course) said that the current keyring implementation sucks, and we should create a keyring object with different implementations which could then be used by client programs, instead of relying on free functions that rely on global configuration.

    I took my time to discuss keyring design and I had little response:

    https://groups.google.com/forum/?fromgroups#!topic/python-keyring/EFl45ddWEPM

    https://bitbucket.org/kang/python-keyring-lib/wiki/Redesign

    (yes, I wrote that last page, if anyone noticed it)

    Hence I think I don't deserve such treatment.

  4. Jason R. Coombs

    I apologize. I did not intend to be negative, but rather report on what I found as part of a more thorough review. I thought by providing details of my findings rather than a terse dismissal would reflect more respect. I do appreciate the contribution and led with that in my discussion. I appreciate your responses and any further contributions you may make. In retrospect, perhaps you're right and I could have closed with a simple message.

    I have noticed the Redesign page and I continue to work to refactor keyring toward a more sensible architecture.

  5. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.