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

Add SelectorObjects wrapper for forward and backward compatibility #598

Merged
merged 14 commits into from
Apr 12, 2023

Conversation

philippjfr
Copy link
Member

@philippjfr philippjfr commented Jan 31, 2022

We have long struggled with an approach to improving Selector, ListSelector and ObjectSelector that would allow users to easily update the objects. Due to unfortunate past decisions this is quite difficult because users can supply both a list or a dict in the constructor and we decided to store objects as a simple list while storing the optional names as a dictionary on the names attribute. This meant that if you instantiated objects as a dictionary you had to manage updates to both of these attributes at the same time.

To handle this issue we create a wrapper object around the objects attribute which has hybrid behavior that allows both list-like and dict-like updates and warns appropriately if for instance you are trying to update objects with the list-like API but have supplied names for other objects previously. An additional benefit of this wrapper object is that if you use any of the list or dict API to modify the objects we can trigger an event, while in the past a user would manually have to trigger such an event if they wanted Panel or some other downstream library to update in response to the change in objects.

Fixes #309
Fixes #331
Fixes #645
Fixes #398

  • Add tests
  • Update docs

@philippjfr philippjfr force-pushed the selector_objects_assign branch 3 times, most recently from ad6fe8c to 65bdc52 Compare January 31, 2022 17:38
@philippjfr
Copy link
Member Author

Will need some internal remapping to allow users to subscribe to parameter:objects despite the fact the slot is now called _objects.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Merging #598 (b6ba65a) into master (7d702ad) will decrease coverage by 1.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #598      +/-   ##
==========================================
- Coverage   81.95%   80.57%   -1.38%     
==========================================
  Files           4        4              
  Lines        3020     3110      +90     
==========================================
+ Hits         2475     2506      +31     
- Misses        545      604      +59     
Impacted Files Coverage Δ
param/parameterized.py 82.20% <100.00%> (+0.02%) ⬆️
numbergen/__init__.py 76.05% <0.00%> (-3.87%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d702ad...b6ba65a. Read the comment docs.

@jbednar jbednar added this to the 2.0 milestone May 16, 2022
@philippjfr philippjfr force-pushed the selector_objects_assign branch from b6ba65a to 4066393 Compare January 8, 2023 15:13
@philippjfr
Copy link
Member Author

@jbednar This is ready for review. I would suggest instead of reviewing the code, review the test cases and check whether they match your understanding and intuitions.

One thing to note is that I've decided to allow dictionary style updates to list-like objects by upgrading the list to a dictionary, i.e. you can do something like this:

>>> p.selector.objects = ['A', 'B', 'C']

>>> p.selector.objects['D'] = 4

>>> dict(p.selector.objects)

{'A': 'A', 'B': 'B', 'C': 'C', 'D': 4}

Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have to admit that my eyes glazed over both on the list/dict methods and on the tests; they all look reasonable but I have low confidence I'd detect any problems. I think we'll have to test this with lots of existing code to see if we find any problems. Have you tried it against the Panel test suite?

examples/user_guide/Parameter_Types.ipynb Outdated Show resolved Hide resolved
param/__init__.py Outdated Show resolved Hide resolved
param/__init__.py Show resolved Hide resolved
philippjfr and others added 2 commits March 12, 2023 18:57
Co-authored-by: James A. Bednar <jbednar@continuum.io>
Co-authored-by: James A. Bednar <jbednar@continuum.io>
@philippjfr
Copy link
Member Author

This is ready to merge once we drop py2.7 on main.

param/__init__.py Outdated Show resolved Hide resolved
@maximlt
Copy link
Member

maximlt commented Apr 5, 2023

Does it fix #398 too?

@philippjfr
Copy link
Member Author

Yes, it absolutely should.

@philippjfr
Copy link
Member Author

Just confirmed that it does.

self.names = objects
self._objects = list(objects.values())
else:
self.names = {}
Copy link
Member

Choose a reason for hiding this comment

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

The default value of names is now an empty dict when objects is a list, instead of None. The test suite fails now, since I added a bunch of tests that check the default values.

Seems like this change was intentional, @philippjfr can you confirm? If so, I can update the tests accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I assumed this was an intentional change and updated the tests accordingly. Can always amend before 2.0 is release if need be.

@maximlt maximlt merged commit 6623a6c into main Apr 12, 2023
@maximlt maximlt deleted the selector_objects_assign branch April 12, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants