-
Notifications
You must be signed in to change notification settings - Fork 58
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
Allow installed extensions to be configured with AsdfConfig #847
Allow installed extensions to be configured with AsdfConfig #847
Conversation
03e1f06
to
c900f3f
Compare
self._extension_list = AsdfExtensionList(self.extensions) | ||
|
||
return self._extension_list | ||
return AsdfExtensionList(self.extensions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exceedingly too clever for me. This class(?) property uses self.extensions, but the class has no init nor inherits from another class, and yet has that attribute? I need to catch up on my meta meta classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extensions property is still there! It's just obscured on GitHub by the ugly diff.
asdf/asdf/extension/_legacy.py
Lines 187 to 190 in c900f3f
@property | |
def extensions(self): | |
from ..config import get_config | |
return [e for e in get_config().extensions if e.legacy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Sorry...
@@ -214,8 +247,10 @@ def _check_extensions(self, tree, strict=False): | |||
return | |||
|
|||
for extension in tree['history']['extensions']: | |||
installed = next((e for e in self.extensions if e.class_name == extension.extension_class), None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I would prefer a bit more verbose construction than using next() to provide the default None. But that's just me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be changing in a future PR anyway, since the condition gets too complex for one line. I'll leave it alone for the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem I saw was my own lying eyes...
This PR refactors the existing extension code to make the new API easier to implement. Changes include:
_DefaultExtensions
to the newasdf.entry_points
module_DefaultExtensions
toAsdfConfig
_DefaultExtensions
so that it becomes a thin interface aroundAsdfConfig