Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] #354

kitchoi · 2020-10-28T18:46:25Z

Closes #291

This PR allows on_trait_change("name_items") to be migrated to observe("name:items") where the name is defined as an ExtensionPoint trait.

After considering all the constraints and alternative approach (see #291), I have come to the conclusion that the support needs to happen by caching the TraitList returned for the ExtensionPoint and making it impossible to mutate the list directly. Having a persisted list that can change is, strictly speaking, what on_trait_change("x_items") or observe("x:items") assumes.

This means the list of extensions has to be maintained not just on the extension registry, but on the object with the extension point as well. This requires fair amount of efforts to keep these values in-sync. This PR still treats the extension registry as the single source of truth as to what extensions are available. The ExtensionPoint remains to be a proxy to query values in an extension registry.

Caution: This implementation requires Traits 6.1, as it imports TraitList from traits.trait_list_object. We should decide if it is necessary to phase in Traits 6.1. Should it be decided that the next release of Envisage will still support Traits 6.0, some additional compatibility layer will be needed for this PR. Edited: ~~but I don't think it would be much (We just need to roll the old ExtensionPoint definition for that, that's all.)~~, or this PR can be considered for merging after next release.

This is achieved by caching the value of ExtensionPoint and make it impossible to mutate the list directly.

envisage/plugin.py

kitchoi · 2020-10-28T18:52:06Z

envisage/tests/test_extension_point_changed.py

-
-        # We shouldn't get a trait event here because we haven't accessed the
-        # extension point yet!
-        self.assertEqual(None, listener.obj)


This test is removed because it is testing the behaviour noted in the "fixme" comment:

# fixme: If the extension point has not been accessed then the # provider extension registry can't work out what has changed, so it # won't fire a changed event.

The "fixme" is now fixed.

kitchoi

I will to check for cycle references here, marking as WIP for a bit.

envisage/extension_point.py

…disconnecting listeners

The reference only serves to support some edge cases where the extension point values become out-of-sync because the listeners are not connected yet. Once the listeners are connected the values are refreshed. So all in all it is not worth keeping the reference, as one will then need to use weakref to avoid cycle references, and special handling in the pickling logic to handle that weakref.

… listeners and trait types

kitchoi · 2020-10-29T11:40:39Z

I will to check for cycle references here, marking as WIP for a bit.

Done.

envisage/extension_point.py

envisage/plugin.py

kitchoi · 2020-11-03T17:18:09Z

From offline discussion: The current plan for the next release of Envisage (current version is 4.9.2) is that it will still support Traits 6.0. With that this PR should remain on hold until after the next release.

Review is still very welcome though.

rahulporuri

This LGTM but I would like to test this against internal apps which use envisage to confirm further.

Also, this is a substantial change and I think it might be good to put together a design document accompanying the changes. Do you think that's worth the time?

rahulporuri · 2020-11-11T10:09:46Z

envisage/extension_point.py

+            warnings.warn(
+                "Extension point cannot be mutated directly.",
+                RuntimeWarning,
+                stacklevel=2,
+            )


is it overkill to refactor all of the warnings into a common _raise_warnings method or something of that sort?

Not an overkill at all... I should do that.

I think it might be good to put together a design document accompanying the changes.

I'd thought that extensive class docstring on _ExtensionPointValue was good enough to explain the rationale, the benefit compared to having a separate design document is that it is close to the code, and therefore it can describe concrete details that can be maintained together. A design document tends to stay high-level and more abstract. Is there any high-level rationale I missed there and does not sit well in the class or module docstring?

that makes sense too. i think class and module docstrings should be enough.

envisage/tests/test_extension_point.py

…list

kitchoi · 2020-11-30T14:16:25Z

Refactored warning code and merged master in here. However the pickability on Python 3.7+ needs a bit more investigation.

…list

kitchoi · 2021-01-06T17:23:34Z

Fixed un/pickling issue. Note that CI is not run against Python 3.7+. I ran the test locally with a venv from Python 3.9 and verified they pass.

rahulporuri

Changes LGTM. Spending a bit of time testing these changes with internal projects.

envisage/extension_point.py

Co-authored-by: Poruri Sai Rahul <[email protected]>

rahulporuri · 2021-01-13T10:41:56Z

Tested against an internal application and things worked as expected. Still LGTM. I'll maybe spend a bit more time testing against one more internal application.

rahulporuri · 2021-01-13T10:46:31Z

Caution: This implementation requires Traits 6.1, as it imports TraitList from traits.trait_list_object. We should decide if it is necessary to phase in Traits 6.1. Should it be decided that the next release of Envisage will still support Traits 6.0, some additional compatibility layer will be needed for this PR. Edited: but I don't think it would be much (We just need to roll the old ExtensionPoint definition for that, that's all.), or this PR can be considered for merging after next release.

I think the next release of envisage will depend on traits >= 6.2.0 so we don't need to worry about adding any additional compatibility layer.

kitchoi · 2021-01-13T11:45:12Z

I think the next release of envisage will depend on traits >= 6.2.0 so we don't need to worry about adding any additional compatibility layer.

I will update the setup.py to require traits>=6.1 so that the bump is tied to the change that requires it.

kitchoi · 2021-01-13T11:56:06Z

I will update the setup.py to require traits>=6.1 so that the bump is tied to the change that requires it.

Done. I believe the setup.py the only place that requires the bump, and I tested installation locally with pip successfully (CI does not have a job that uses pip install with the local source). I did not change etstool.py as the EDM tool is already pulling in the latest released version of traits, and if it did not, the test suite will fail so we will know about it.

kitchoi · 2021-01-14T16:03:36Z

Confirmed offline that this can be merged... merging...

…aits 6.1] (#354)" This reverts commit d62af08.

* Revert "Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] (#354)" This reverts commit d62af08. * TST/FIX : Revert test that used observe to listen to changes to extension point items. The test now uses a static trait change handler like before modified: envisage/tests/test_plugin.py

* Revert PR #354 (#422) * Revert "Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] (#354)" This reverts commit d62af08. * TST/FIX : Revert test that used observe to listen to changes to extension point items. The test now uses a static trait change handler like before modified: envisage/tests/test_plugin.py * Regression test for #417 (#421) * Add failing test * STY: Fix line spacing issues Co-authored-by: Poruri Sai Rahul <[email protected]> * Fix test suite dependence on ipykernel (#423) * Fix hard test suite dependence on ipykernel * Update envisage/tests/test_ids.py Co-authored-by: Poruri Sai Rahul <[email protected]> Co-authored-by: Poruri Sai Rahul <[email protected]> * DOC : Update changelog modified: CHANGES.rst Co-authored-by: Mark Dickinson <[email protected]>

Support @observe("name:items") for ExtensionPoint

b3ced00

This is achieved by caching the value of ExtensionPoint and make it impossible to mutate the list directly.

kitchoi commented Oct 28, 2020

View reviewed changes

envisage/plugin.py Outdated Show resolved Hide resolved

kitchoi commented Oct 28, 2020

View reviewed changes

kitchoi commented Oct 29, 2020

View reviewed changes

envisage/extension_point.py Outdated Show resolved Hide resolved

envisage/extension_point.py Outdated Show resolved Hide resolved

kitchoi changed the title ~~Support observe("name:items") for ExtensionPoint [Requires Traits 6.1]~~ [WIP] Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] Oct 29, 2020

kitchoi added 5 commits October 29, 2020 09:50

Make sure an object with ExtensionPoint is garbage collectable after …

ab81199

…disconnecting listeners

Clean up test and add docstring

4a2a27f

Fix comment after removing query method reimplementation

723ef9c

Move functions out of ExtensionPoint to avoid cycle reference between…

fc880be

… listeners and trait types

kitchoi changed the title ~~[WIP] Support observe("name:items") for ExtensionPoint [Requires Traits 6.1]~~ Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] Oct 29, 2020

rahulporuri reviewed Nov 3, 2020

View reviewed changes

envisage/extension_point.py Outdated Show resolved Hide resolved

envisage/extension_point.py Show resolved Hide resolved

envisage/plugin.py Outdated Show resolved Hide resolved

Improve docstring for exent

208a055

rahulporuri reviewed Nov 11, 2020

View reviewed changes

aaronayres35 mentioned this pull request Nov 23, 2020

Fix index slice in ExtensionPointChangedEvent when plugin changes #357

Merged

kitchoi commented Nov 30, 2020

View reviewed changes

envisage/tests/test_extension_point.py Show resolved Hide resolved

kitchoi added 2 commits November 30, 2020 14:07

Merge branch 'master' into 291-extension-point-as-a-cached-immutable-…

2b3ad7d

…list

Refactoring warning

e998fbe

kitchoi added 4 commits January 6, 2021 16:35

Merge branch 'master' into 291-extension-point-as-a-cached-immutable-…

76dcda0

…list

Fix unpickling ExtensionPointValue in newer versions of Python

b6dfe8e

Add a comment for the stand-in replacement

3765d1e

Restore comment location

b2166e5

Restore stacklevel in warning

6f412ce

rahulporuri self-requested a review January 12, 2021 16:16

rahulporuri approved these changes Jan 13, 2021

View reviewed changes

envisage/extension_point.py Outdated Show resolved Hide resolved

Remove a redundant back :tick:

08f48bc

Co-authored-by: Poruri Sai Rahul <[email protected]>

kitchoi mentioned this pull request Jan 13, 2021

Re-export TraitList/TraitSet/TraitDict containers (not handlers) in API? enthought/traits#1332

Open

Bump traits requirement to 6.1

c34c15f

kitchoi merged commit d62af08 into master Jan 14, 2021

kitchoi deleted the 291-extension-point-as-a-cached-immutable-list branch January 14, 2021 16:04

mdickinson mentioned this pull request Jun 8, 2021

Extension point resolution too eager in Envisage 6.0.0 #417

Closed

rahulporuri pushed a commit that referenced this pull request Jun 8, 2021

Revert "Support observe("name:items") for ExtensionPoint [Requires Tr…

1fc9865

…aits 6.1] (#354)" This reverts commit d62af08.

rahulporuri pushed a commit that referenced this pull request Jun 8, 2021

Revert "Support observe("name:items") for ExtensionPoint [Requires Tr…

0d04ec6

…aits 6.1] (#354)" This reverts commit d62af08.

rahulporuri mentioned this pull request Jun 8, 2021

Revert PR #354 #418

Closed

kitchoi mentioned this pull request Jun 8, 2021

WIP: Remove eagerness to evaluate extension points. #419

Closed

rahulporuri pushed a commit that referenced this pull request Jun 11, 2021

Revert "Support observe("name:items") for ExtensionPoint [Requires Tr…

585f16d

…aits 6.1] (#354)" This reverts commit d62af08.

rahulporuri mentioned this pull request Jun 11, 2021

Revert PR #354 #422

Merged

mdickinson mentioned this pull request Feb 6, 2023

Support for observe in ExtensionPoint #291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] #354

Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] #354

kitchoi commented Oct 28, 2020 •

edited

Loading

kitchoi Oct 28, 2020

kitchoi left a comment

kitchoi commented Oct 29, 2020

kitchoi commented Nov 3, 2020

rahulporuri left a comment

rahulporuri Nov 11, 2020

kitchoi Nov 11, 2020

rahulporuri Nov 11, 2020

kitchoi commented Nov 30, 2020

kitchoi commented Jan 6, 2021

rahulporuri left a comment

rahulporuri commented Jan 13, 2021

rahulporuri commented Jan 13, 2021

kitchoi commented Jan 13, 2021

kitchoi commented Jan 13, 2021 •

edited

Loading

kitchoi commented Jan 14, 2021

Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] #354

Support observe("name:items") for ExtensionPoint [Requires Traits 6.1] #354

Conversation

kitchoi commented Oct 28, 2020 • edited Loading

kitchoi Oct 28, 2020

Choose a reason for hiding this comment

kitchoi left a comment

Choose a reason for hiding this comment

kitchoi commented Oct 29, 2020

kitchoi commented Nov 3, 2020

rahulporuri left a comment

Choose a reason for hiding this comment

rahulporuri Nov 11, 2020

Choose a reason for hiding this comment

kitchoi Nov 11, 2020

Choose a reason for hiding this comment

rahulporuri Nov 11, 2020

Choose a reason for hiding this comment

kitchoi commented Nov 30, 2020

kitchoi commented Jan 6, 2021

rahulporuri left a comment

Choose a reason for hiding this comment

rahulporuri commented Jan 13, 2021

rahulporuri commented Jan 13, 2021

kitchoi commented Jan 13, 2021

kitchoi commented Jan 13, 2021 • edited Loading

kitchoi commented Jan 14, 2021

kitchoi commented Oct 28, 2020 •

edited

Loading

kitchoi commented Jan 13, 2021 •

edited

Loading