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 index slice in ExtensionPointChangedEvent when plugin changes #357

Merged
merged 2 commits into from
Nov 24, 2020

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Nov 3, 2020

When the extensions list is changed by adding or removing a plugin, the ExtensionPointChangedEvent reports a slice object as the index, but the end value is incorrect: If one uses that index to reproduce the new list from the values of the old list, they get incorrect results.

The first commit adds two assertions. The first assertion fails with master like this:

Traceback (most recent call last):
  File "/Users/kchoi/Work/ETS/envisage/envisage/tests/test_extension_point_changed.py", line 268, in test_assign_non_empty_list
    listener.new.removed, source_values[listener.new.index]
AssertionError: [1, 2, 3] != [1, 2, 3, 98]

That is, if you use the index on the old values to recover the removed values, you get the wrong answer.

The second assertion fails with master like this:

Traceback (most recent call last):
  File "/Users/kchoi/Work/ETS/envisage/envisage/tests/test_extension_point_changed.py", line 274, in test_assign_non_empty_list
    self.assertEqual(source_values, application.get_extensions("a.x"))
AssertionError: Lists differ: [2, 4, 6, 8, 99, 100] != [2, 4, 6, 8, 98, 99, 100]

First differing element 4:
99
98

Second list contains 1 additional elements.
First extra element 6:
100

- [2, 4, 6, 8, 99, 100]
+ [2, 4, 6, 8, 98, 99, 100]
?             ++++

That is if you use the index on the old values and assign the "added" values to it, you also don't recover the new values.

The second commit fixes both assertions, and make the necessary change to the test that asserts the slice end value.

@aaronayres35
Copy link
Contributor

Do we want to try to push this through for the 5.0.0 release, or is it intended to wait like #354?

@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 23, 2020

I don't think this needs to wait like #354. @rahulporuri what do you think?

@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 23, 2020

On the other hand, if this bug has lived this long (13 years) and no one realized it, it is probably not important enough for it to block the upcoming release.

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's push this through in this release.

@rahulporuri
Copy link
Contributor

@kitchoi sorry for the enormous delay on this PR.

@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 24, 2020

No problem at all. Thank you!

@kitchoi kitchoi merged commit 27f948f into master Nov 24, 2020
@kitchoi kitchoi deleted the fix-extension-index branch November 24, 2020 10:26
@kitchoi
Copy link
Contributor Author

kitchoi commented Nov 25, 2020

I believe this has not gone into the maintenance branch, so I am deploying the 'need backport to 5.0' tag.

aaronayres35 added a commit that referenced this pull request Dec 2, 2020
* Fix index slice in ExtensionPointChangedEvent when plugin changes (#357)

* Turn off macOS builds on Travis CI (#375)

This PR turns off expensive macOS builds on Travis CI. We'll eventually use GitHub Actions to replace these.

* update changelog with backported PRs

* Make example run from any directory (#377)

* Make it possible to run example scripts from anywhere.

* Add a docstring for the test case

* Add missing __init__.py

* Import abcdefg...

* Refactor documentation links to source on GitHub (#379)

* Refactor external links to demo examples from extension_points.rst

* Update links to github in introduction.rst

* Refactor substitutions

* Group substitutions

* Update other references to github links

* Flake8

* Remove two other links to github that point to TraitsGUI

* Remove redundant newlines

* Maintain all substituions in the same place

* Remove two redundant lines

* Contribute examples to etsdemo (#380)

* remove logging from hello world and motd

* remove logging from other examples

* move examples into envisage

* update the single URL in docs/conf.py

* add examples as package_data

* add entry point

* flake8

* add demo folder and update package_data based off code review suggestion

* Update docs/source/conf.py

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* rename and add test for entry point

* move GUI_Application example into envisage/examples

* move old examples into envisage/examples/demo/legacy

* Revert "move old examples into envisage/examples/demo/legacy"

This reverts commit c026040.

* move old examples into a new subdirectory called legacy

* add a readme to reference new location for examples

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* more changelog updates

* more changes to changelog

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
aaronayres35 added a commit that referenced this pull request Dec 4, 2020
* Fix index slice in ExtensionPointChangedEvent when plugin changes (#357)

* Turn off macOS builds on Travis CI (#375)

This PR turns off expensive macOS builds on Travis CI. We'll eventually use GitHub Actions to replace these.

* update changelog with backported PRs

* Make example run from any directory (#377)

* Make it possible to run example scripts from anywhere.

* Add a docstring for the test case

* Add missing __init__.py

* Import abcdefg...

* Refactor documentation links to source on GitHub (#379)

* Refactor external links to demo examples from extension_points.rst

* Update links to github in introduction.rst

* Refactor substitutions

* Group substitutions

* Update other references to github links

* Flake8

* Remove two other links to github that point to TraitsGUI

* Remove redundant newlines

* Maintain all substituions in the same place

* Remove two redundant lines

* Contribute examples to etsdemo (#380)

* remove logging from hello world and motd

* remove logging from other examples

* move examples into envisage

* update the single URL in docs/conf.py

* add examples as package_data

* add entry point

* flake8

* add demo folder and update package_data based off code review suggestion

* Update docs/source/conf.py

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* rename and add test for entry point

* move GUI_Application example into envisage/examples

* move old examples into envisage/examples/demo/legacy

* Revert "move old examples into envisage/examples/demo/legacy"

This reverts commit c026040.

* move old examples into a new subdirectory called legacy

* add a readme to reference new location for examples

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* more changelog updates

* more changes to changelog

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
aaronayres35 added a commit that referenced this pull request Dec 4, 2020
* Backports for 5.0.0 and update changelog (#378)

* Fix index slice in ExtensionPointChangedEvent when plugin changes (#357)

* Turn off macOS builds on Travis CI (#375)

This PR turns off expensive macOS builds on Travis CI. We'll eventually use GitHub Actions to replace these.

* update changelog with backported PRs

* Make example run from any directory (#377)

* Make it possible to run example scripts from anywhere.

* Add a docstring for the test case

* Add missing __init__.py

* Import abcdefg...

* Refactor documentation links to source on GitHub (#379)

* Refactor external links to demo examples from extension_points.rst

* Update links to github in introduction.rst

* Refactor substitutions

* Group substitutions

* Update other references to github links

* Flake8

* Remove two other links to github that point to TraitsGUI

* Remove redundant newlines

* Maintain all substituions in the same place

* Remove two redundant lines

* Contribute examples to etsdemo (#380)

* remove logging from hello world and motd

* remove logging from other examples

* move examples into envisage

* update the single URL in docs/conf.py

* add examples as package_data

* add entry point

* flake8

* add demo folder and update package_data based off code review suggestion

* Update docs/source/conf.py

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* rename and add test for entry point

* move GUI_Application example into envisage/examples

* move old examples into envisage/examples/demo/legacy

* Revert "move old examples into envisage/examples/demo/legacy"

This reverts commit c026040.

* move old examples into a new subdirectory called legacy

* add a readme to reference new location for examples

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>

* more changelog updates

* more changes to changelog

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>

* add release date

Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants