Add RangeEditor support for format_func #1684

aaronayres35 · 2021-06-11T21:44:34Z

This PR adds support for format_func for the RangeEditor. It does so by calling the string_value method defined on the factory, rather than just self.format % _____. See method definition here:

traitsui/traitsui/editor_factory.py

Lines 190 to 208 in e2521b4

    
               def string_value(self, value, format_func=None): 
        
                   """ Returns the text representation of a specified object trait value. 
        
                   If the **format_func** attribute is set on the editor factory, then 
        
                   this method calls that function to do the formatting.  If the 
        
                   **format_str** attribute is set on the editor factory, then this 
        
                   method uses that string for formatting. If neither attribute is 
        
                   set, then this method just calls the appropriate text type to format. 
        
                   """ 
        
                   if self.format_func is not None: 
        
                       return self.format_func(value) 
        
                   if self.format_str != "": 
        
                       return self.format_str % value 
        
                   if format_func is not None: 
        
                       return format_func(value) 
        
                   return str(value)

I am just realizing while opening this that although the base EditorFactory class defines a format_str trait, the RangeEditor defines its own format trait to hold the format string instead. ~~I suspect this will cause problems, hence the WIP for now. I would like to rename format to format_str but that is part of the public api...~~
~~Perhaps I will need to redefine the string_value method on RangeEditor and have it be exactly the same as the base class just replacing format_str with format... 🤔~~

~EDIT: I went with the approach mentioned of "redefine the string_value method on RangeEditor ...". It smells a bit, but seems necessary (?) I am not sure why RangeEditor chose to use its own trait there previously rather than using format_str. ~

This PR also went on to deprecate the format trait on the RangeEditor EditorFactory and also on the toolkit specific Editor implementations. Users should transition to using format_str and format_func on the editor factory. The format trait will be removed in traitsui 8.0 (see #1704)

Checklist

Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

…should be using format not format_str in this casee

rahulporuri

This approach works but I would like to see the alternative approach before choosing one over the other.

IMO, in the long term, it would be better if we deprecated and remove RangeEditor.format and just use EditorFactory.format_str. The right way to do that at the moment would be to make RangeEditor.format a property that sets RangeEditor.format_str while raising a deprecation warning.

rahulporuri · 2021-06-21T08:59:41Z

traitsui/editors/range_editor.py

+        method uses that string for formatting. If neither attribute is
+        set, then this method just calls the appropriate text type to format.


There is no mention of the format_func kwarg to the method in the docstring here.

Tangentially, the priority here seems a little wonky to me - shouldn't format_func passed to the method take priority over the format_func/format set on the factory? I think it should because format_func is being explicitly passed when calling string_value - and format_func/format might have been set on the factory at any earlier point after instantiation.

I agree, this is quite strange and unexpected behavior...
I will open an issue pointing to the origin of this method (i.e. EditorFactory.string_value)

I've opened #1695

aaronayres35 · 2021-06-21T15:20:41Z

traitsui/qt4/range_editor.py

    high = Any()

-    #: Formatting string used to format value and labels
+    #: Deprecated: This trait is no longer used.


I was thinking this should become a property that mirrors the editors factory_str but I don't think that is needed / allowed? (ie an instance of an editor can't set its factory's trait). We ultimately had previously just set self.format = factory.format in the init method anyways.

I am worried there may be a change in behavior if a user was to try to set format on an instance of a RangeEditor. e.g my_editor = RangeEditor(), my_editor.format = "something". Previously, because we had been using self.format throughout instead of string_value, so this would actually be used. Now, AFAICT, the best we can do is pass it as a kwarg to string_value if a user tries to set it on an instance, but that will no longer take priority if the factory has its trait set...

I don't think we want to change any behavior here (even if the current behavior is strange). I am unsure how to best move forward.

@rahulporuri any thoughts on this? I hope my explanation made some sense, if not we can discuss tomorrow at standup

So initially I expected this Test to pass on master, but fail on this branch:

def test_format_behavior_change(self): # format trait has been deprecated in favor of format_str. However, # behavior should be unchanged. model = RangeModel() with self.assertWarns(DeprecationWarning): my_editor = RangeEditor(format="%s ...") my_editor.format = "%s" view = View( Item( "float_value", editor=my_editor ) ) tester = UITester() with tester.create_ui(model, dict(view=view)) as ui: float_value_field = tester.find_by_name(ui, "float_value") float_value_text = float_value_field.locate(Textbox()) self.assertEqual( float_value_text.inspect(DisplayedText()), "0.1" )

but it passes on both. I then realized my_editor is always just an instance of the traitsui.editors.RangeEditor EditorFactory. The actual Editor instances are only created when e.g. the _get_simple_editor_class method is called. If I access the Editor directly, e.g. doing something like this:

def test_format_behavior_change(self): # format trait has been deprecated in favor of format_str. However, # behavior should be unchanged. model = RangeModel() view = View( Item( "float_value", editor=RangeEditor(format="%s ...") ) ) tester = UITester() with tester.create_ui(model, dict(view=view)) as ui: float_value_field = tester.find_by_name(ui, "float_value") float_value_field._target.format = "%s" float_value_text = float_value_field.locate(Textbox()) self.assertEqual( float_value_text.inspect(DisplayedText()), "0.1" )

I expected it to pass on master and fail on this branch. However, this fails on both...
So, AFAICT, behavior is not changed by this PR.

EDIT: see the recently added test

Can you search for users of the format trait on this editor? I'm not even sure how one would go about specifically searching for such uses. Whether or not users exist would help us decide on whether or not we need to convert format into a property as well - and make getting/setting format raise warnings as well.

I was unable to find any users trying to set format on the Editor instance directly.
I did see users setting it on the Factory object, ie RangeEditor(format= ...), but that is covered by the use of property/deprecation in traitsui/editors/range_editor.py.

TBH I am not certain what the pattern would be to set it on the Editor instance. I guess accessing it through UIInfo?

From traitsUI docs: "Note that not every trait on every editor can react to changes: some values are only used at editor creation time; however all editors support dynamically changing the enabled, visible and invalid traits. This feature can sometimes allow developers to avoid having to create a custom Handler subclass."

… the factory anyway

aaronayres35 · 2021-06-22T20:47:44Z

traitsui/wx/range_editor.py

    high = Any()

-    #: Formatting string used to format value and labels
+    #: Deprecated: This trait is no longer used.  


Rather than simply commenting on the deprecation, I would like to make this a property which raises a deprecation warning.
Ideally, getting the property would return the self.factory.format_str trait, and like wise setting it would set that trait. However it feels incorrect for an editor instance to be trying to change a trait on its factory...

Also as I think more about this PR, I am worried about Hyrum's law here. Even if the behavior change seems "more" correct, it is possible that users are dependent on the current behavior.

rahulporuri

LGTM with a couple of comments.

Can you update the PR description as well? I think it is outdated as we modified the approach after the first review.

traitsui/editors/range_editor.py

rahulporuri · 2021-06-28T11:27:36Z

traitsui/qt4/range_editor.py

    high = Any()

-    #: Formatting string used to format value and labels
+    #: Deprecated: This trait is no longer used.


Can you search for users of the format trait on this editor? I'm not even sure how one would go about specifically searching for such uses. Whether or not users exist would help us decide on whether or not we need to convert format into a property as well - and make getting/setting format raise warnings as well.

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

aaronayres35 · 2021-06-28T15:34:05Z

I've updated the news fragment, although it is still a "bugfix". It really sort of belongs in "deprecation" as well... Should I include it in both (ie add another news fragment with the same message just tagged as deprecation perhaps?

aaronayres35 · 2021-06-28T18:16:42Z

CI is failing on ubuntu-latest with pyside2:

Command '['edm', 'run', '-e', 'traitsui-test-3.6-pyside2', '--', 'python', '-W', 'default', '-m', 'unittest', 'discover', '-v', '/home/runner/work/traitsui/traitsui/integrationtests']' returned non-zero exit status 252.
/usr/bin/bash /home/runner/work/_actions/GabrielBB/xvfb-action/v1/cleanup.sh
No xvfb processes to kill
Error: The process '/usr/bin/xvfb-run' failed with exit code 1

~~This is also occurring on #1705~~EDIT: it has now stopped occurring on #1705, and I can not reproduce locally... Seems to be an intermittent failure.
And it seems to have first occurred on #1693

I have a suspicion that these failures occur when we have CI running for multiple PRs at the same time. This is really a hunch, but with some quick googling I have seen some discussions about xvfb-run failing and then being fixed by adding a -a flag "so xvfb uses another display if __ is in use.". And it seems like this PR is going to pass now after I just re-triggered CI again (only this time no other PR is running CI). This could very well be entirely wrong though.

rahulporuri

still LGTM

rahulporuri · 2021-06-29T13:33:47Z

@aaronayres35 can you update the PR description because it feels outdated

aaronayres35 added 8 commits June 11, 2021 16:22

add a currently failing test

dd82eec

remove call to configure_traits

cf42ce4

make test pass on qt by using the string_value method on the factory

d25a17f

make same fix on wx

fc9ae39

redefine string_value method on RangeEditor to preserve behavior. We …

3032065

…should be using format not format_str in this casee

add a test for use of format trait to ensure we aren't changing behavior

017714b

fix test and string_value method (missed one format_str)

7383f01

add news fragment

9ad59b8

aaronayres35 changed the title ~~[WIP] Support for format_func with Range editor~~ Add RangeEditor support for format_func Jun 11, 2021

rahulporuri self-requested a review June 14, 2021 13:14

rahulporuri reviewed Jun 21, 2021

View reviewed changes

deprecate format trait and make it an alias for format_str

bb7a3c4

aaronayres35 commented Jun 21, 2021

View reviewed changes

add deprecation comment on wx

f4fc4b0

rahulporuri self-requested a review June 22, 2021 13:15

aaronayres35 added 2 commits June 22, 2021 15:29

add a test demonstrating behavior change

a7304b0

call string_value on the editor as this ends up calling the method on…

cd26554

… the factory anyway

aaronayres35 commented Jun 22, 2021

View reviewed changes

rahulporuri approved these changes Jun 28, 2021

View reviewed changes

aaronayres35 and others added 2 commits June 28, 2021 04:40

Apply suggestions from code review

68c42f1

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

update news fragment

d618dde

aaronayres35 mentioned this pull request Jun 28, 2021

Remove deprecated format trait on RangeEditor / implementations #1704

Open

add comment refering to new issue

2bf3fa5

aaronayres35 requested a review from rahulporuri June 29, 2021 13:12

This comment has been minimized.

Sign in to view

rahulporuri approved these changes Jun 29, 2021

View reviewed changes

aaronayres35 merged commit 347f0b5 into master Jun 29, 2021

aaronayres35 deleted the range-editor-format_func branch June 29, 2021 13:28

ericpre mentioned this pull request Apr 18, 2022

Fix deprecation warning traitsui hyperspy/hyperspy_gui_traitsui#45

Merged

4 tasks

	def string_value(self, value, format_func=None):
	""" Returns the text representation of a specified object trait value.

	If the format_func attribute is set on the editor factory, then
	this method calls that function to do the formatting. If the
	format_str attribute is set on the editor factory, then this
	method uses that string for formatting. If neither attribute is
	set, then this method just calls the appropriate text type to format.
	"""
	if self.format_func is not None:
	return self.format_func(value)

	if self.format_str != "":
	return self.format_str % value

	if format_func is not None:
	return format_func(value)

	return str(value)

Add RangeEditor support for format_func #1684

Add RangeEditor support for format_func #1684

Uh oh!

Conversation

aaronayres35 commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronayres35 commented Jun 28, 2021

Uh oh!

aaronayres35 commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

rahulporuri commented Jun 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aaronayres35 commented Jun 11, 2021 •

edited

Loading

aaronayres35 commented Jun 28, 2021 •

edited

Loading