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

Replace use of depends_on in Traits Properties with observe #630

Merged
merged 2 commits into from
Apr 8, 2021

Conversation

aaronayres35
Copy link
Contributor

This PR replaces almost all uses of depends_on with observe, aside from this:

index_range = Property(depends_on="index_mapper.range")

changing this line resulted in he following errors:

======================================================================
FAIL: test_linescatter_1d_set_index_mapper_notifies_index_range (chaco.tests.test_line_scatterplot.LineScatterPlot1DTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aayres/Desktop/chaco/chaco/tests/test_line_scatterplot.py", line 185, in test_linescatter_1d_set_index_mapper_notifies_index_range
    self.linescatterplot.index_mapper = LinearMapper(range=new_range)
  File "/Users/aayres/Desktop/traits/traits/testing/unittest_tools.py", line 118, in __exit__
    raise self.failureException(msg.format(*items))
AssertionError: Change event for index_range was fired 2 times instead of 1

======================================================================
FAIL: test_scatter_1d_set_index_mapper_notifies_index_range (chaco.tests.test_scatterplot_1d.Scatterplot1DTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aayres/Desktop/chaco/chaco/tests/test_scatterplot_1d.py", line 181, in test_scatter_1d_set_index_mapper_notifies_index_range
    self.scatterplot.index_mapper = LinearMapper(range=new_range)
  File "/Users/aayres/Desktop/traits/traits/testing/unittest_tools.py", line 118, in __exit__
    raise self.failureException(msg.format(*items))
AssertionError: Change event for index_range was fired 2 times instead of 1

======================================================================
FAIL: test_text_1d_set_index_mapper_notifies_index_range (chaco.tests.test_text_plot_1d.TextPlot1DTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/aayres/Desktop/chaco/chaco/tests/test_text_plot_1d.py", line 133, in test_text_1d_set_index_mapper_notifies_index_range
    self.textplot.index_mapper = LinearMapper(range=new_range)
  File "/Users/aayres/Desktop/traits/traits/testing/unittest_tools.py", line 118, in __exit__
    raise self.failureException(msg.format(*items))
AssertionError: Change event for index_range was fired 2 times instead of 1

----------------------------------------------------------------------

Note that the trait is changed twice with observe. assertTraitChanges conveniently provides the events that were triggered. Printing these for one of the tests, we see:

[
(<chaco.line_scatterplot_1d.LineScatterPlot1D object at 0x7fcfc7e7c258>, 'index_range', <undefined>, <chaco.data_range_1d.DataRange1D object at 0x7fcfc7e7c308>),
(<chaco.line_scatterplot_1d.LineScatterPlot1D object at 0x7fcfc7e7c258>, 'index_range', <undefined>, <chaco.data_range_1d.DataRange1D object at 0x7fcfc7e7c308>)
]

ie the exact same event is occurring twice.

I spent a bit of time trying to understand this, but still am not sure where the problem is coming from.
For now, I figure we can go ahead with the rest of the changes and save this for another day.

Additionally, this PR also changes a property to observe selected_mask not selection_mask as no selection_mask is defined on the class but selected_mask is (I assume this was the original intention, +1 for observe catching a potential silent bug).

@aaronayres35
Copy link
Contributor Author

One thing I am now considering is that it is possible other changes are having a similar effect (triggering a trait to be changed twice), but may just not be explicitly tested. I do not know if this behavior is really problematic or not

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

@aaronayres35 aaronayres35 merged commit fdd858a into master Apr 8, 2021
@rahulporuri rahulporuri deleted the depends_on-to-observe branch April 19, 2021 12:51
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.

2 participants