-
Notifications
You must be signed in to change notification settings - Fork 28
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
API for testing whether persistence is available #92
Comments
Is this pattern adequate? |
I already do that. The trouble is that although azure-identity speculatively instantiates LibsecretPersistence and anticipates failure, msal-extensions logs this error during the attempt. An application developer unfamiliar with the implementation details of azure-identity may take that error to mean azure-identity requires PyGObject (default To be clear, I think it's reasonable to log this message when the caller expects LibsecretPersistence to work. However, a mid-tier library like azure-identity doesn't necessarily expect it to work and can't express that expectation because libsecret.py logs the error on import. So I'm imagining an API like: if LibsecretPersistence.supported():
# instantiate LibsecretPersistence because I have reason to believe it might work
else:
# LibsecretPersistence definitely won't work, don't bother trying it A reasonable alternative to adding such an API is to just not log the error. You could instead raise an exception with the same message in |
Thanks for the explanation! That makes sense. Looks like I would need to refrain from the "log-and-reraise" pattern from now on. Regarding the solution, I like your alternative proposal better, because it does not need to invent yet another new and non-standardized API. How about this fix in MSAL EX? try:
import gi
except ImportError:
long_message = "Runtime dependency of PyGObject is missing. Yada yada yada..."
raise ImportError(long_message) The side effect is the original exception's message and error trace were both lost. This could be considered as a good encapsulation, depends on whom you ask. It won't make much difference in this particular case, though. |
I like the alternative better too. You can preserve the original exception on Python 3 in a 2.7-compatible way: import six
try:
import gi
except ImportError as ex:
message = "Runtime dependency of PyGObject is missing. Yada yada yada..."
six.raise_from(ex, ImportError(message)) But that's just an FYI and another reason to look forward to dropping 2.7 support. On 2.7 I suppose the "raise instead of log" pattern could apply to |
Drop Python 2 to utilize Exception ChainingBeing able to utilize Python 3's Exception Chaining Using
|
To be precise, Python 3's exception chaining would already happen automatically. Quoted from its document: "Exception chaining happens automatically when an exception is raised inside an except or finally section." The rare time we would consider that It is, however, still true that we would eventually migrate to Python 3, in order to have that "automatic exception chaining" even when our code base does NOT contain |
Per my understanding, there is some difference between implicit/automatic exception chaining and explicit This is discussed in more detail in https://www.python.org/dev/peps/pep-3134/
while implicit exception chaining is more like something wrong/unexpected happened during the exception handling logic:
Anyway, this nuance won't affect the logic, as long as MSAL raises the original exception, instead of logging it. |
It would be helpful to have an API for silently testing whether a persistence method is available, in particular LibsecretPersistence. The best way today is to attempt to instantiate the persistence, which logs an error on Linux when PyGObject isn't installed:
It's a reasonable thing to log but an application developer using msal-extensions through another library may interpret it to mean that mid-tier library requires PyGObject (see e.g. Azure/azure-sdk-for-python/issues/12152 and Azure/azure-sdk-for-python/issues/19857).
The text was updated successfully, but these errors were encountered: