Add RangeEditor support for format_func

[aaronayres35]

closes #1650

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)

[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]

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.

[aaronayres35]

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)

[aaronayres35]

I've opened #1695

[aaronayres35]

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

[aaronayres35]

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.

[aaronayres35]

EDIT: see the recently added test

[rahulporuri]

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.

[aaronayres35]

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?

[aaronayres35]

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."

[aaronayres35]

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.

[rahulporuri]

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.

[aaronayres35]

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]

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 #1705EDIT: 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]

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