Pyface imports from non api modules

[aaronayres35]

Ref: enthought/pyface#866

With traits 6.2 we are explicitly stating in documentation that imports should come from api modules. Because of this I decided to run through pyface for any non api imports and this is the list I found:

From traits.trait_base there are imports of:

• [ ] xgetattr (see Expose xgetattr and xsetattr in traits API #1239)
• [ ] xsetattr (see Expose xgetattr and xsetattr in traits API #1239)
• get_resource_path
• user_name_for
• traits_home

Additionally we import optional dependencies from traits.testing.optional_dependencies (pyface shouldn't do this)

From traits.trait_list_object we import TraitsList but this is a different TraitList from the one exposed in traits.api

From traits.version we import version

From traits.util.resource we import get_path

From traitst.trait_notifiers we import set_ui_handler and ui_handler

From traitss.util.clean_strings we import python_name

and from traits.util.camel_case we import camel_case_to_words

Some of these we may want to expose in an api module since they are being used? Others we may want to discourage their use in pyface since traits isn't meant to be a storage place for miscellaneous utilities. Or we may want to just leave them alone and break our own suggestion.

[rahulporuri]

The question of exposing xgetattr and xsetattr has been resolved in - #1239 - and the answer is no.

[aaronayres35]

The question of exposing xgetattr and xsetattr has been resolved in - #1239 - and the answer is no.

Also ref: enthought/ets#51 (this issue may be a bit redundant from the ideas of that issue, but this issue does add an explicit list of imports at least)

[mdickinson]

Additionally we import optional dependencies from traits.testing.optional_dependencies

I'd recommend not doing this: that code is really just a convenience for Traits itself, and it could easily change without reference to external packages.

[rahulporuri]

From traits.version we import version

I don't think it makes sense to expose the version from the api module - because we already expose it as __version__ via traits. So i think we can cross this off as well.

[aaronayres35]

From traits.version we import version

I don't think it makes sense to expose the version from the api module - because we already expose it as __version__ via traits. So i think we can cross this off as well.

Just to clarify pyface should be importing traits and then using traits.__version__ then instead of from traits.version import version?

[rahulporuri]

Just to clarify pyface should be importing traits and then using traits.version then instead of from traits.version import version?

Yeah. I would probably do from traits import __version__ as version. I'm not sure why we were doing from traits.version import version in the first place. This is probably a personal choice though and I can be convinced otherwise.

[mdickinson]

Some of these we may want to expose in an api module since they are being used?

So as a general principle, I don't want Traits to become a dumping ground (any more than it already is) for utilities that aren't explicitly Traits related. If there's a string-handling utility that's actually related to the way that Traits does something in particular, and that Traits-using libraries need to know about, then that's fine (though in that case you'd expect it to have a name that also makes it clear what Traits role is), but general utilities shouldn't be in Traits. Also, anything that is newly exposed in the Traits api should have tests - we shouldn't expose it without those tests.

• get_resource_path

This uses evil magic and its use should be avoided if at all possible. Pyface should find a better way of doing whatever it's doing. It's not used within Traits anywhere, and it's not tested anywhere.

• user_name_for

Not Traits-related, not used within Traits, not tested. Should not be moved into any of the api modules.

• traits_home

What's Pyface using this for? There isn't really a meaningful "Traits home directory".

From traits.trait_list_object we import TraitsList but this is a different TraitList from the one exposed in traits.api

Yep, that one's problematic - we want to make that TraitList available, but can't until the old TraitList is gone. I think we already have an issue for this.

From traits.version we import version

I'd recommend that packages check traits.__version__ instead. (But again, checking the version is a bit of an antipattern - ideally, you check directly for whichever version-sensitive feature you want instead. Why is Pyface using this?)

From traits.util.resource we import get_path

Not used within Traits, not tested. Possibly redundant with post-Python-3.6 path handling capabilities. What's Pyface using this one for? If it is a cross-ETS need, we could potentially make it available, but it would need tests and a clear use-case.

From traits.trait_notifiers we import set_ui_handler and ui_handler

These ones may need to be exposed (but not without thought and an understanding of the use-case - if we're going to expose them, we should also take the opportunity to decide whether there's a better API).

From traitss.util.clean_strings we import python_name

Urgh. Not obviously Traits-related, not used in Traits, not tested. Would need a clear Traits-related use-case to be considered for addition to *.api.

and from traits.util.camel_case we import camel_case_to_words

Ditto.

[mdickinson]

Re: TraitList

I think we already have an issue for this.

#1332

[mdickinson]

Just to clarify pyface should be importing traits and then using traits.__version__ then instead of from traits.version import version?

Well, first you should back up a step. Why does Pyface need the Traits version?

[aaronayres35]

• get_resource_path

This uses evil magic and its use should be avoided if at all possible. Pyface should find a better way of doing whatever it's doing. It's not used within Traits anywhere, and it's not tested anywhere.

I've opened enthought/pyface#869

• traits_home

What's Pyface using this for? There isn't really a meaningful "Traits home directory".

https://github.com/enthought/pyface/blob/2974f580c4c10ec4142641ca67b15ec27593c5ae/pyface/dock/dock_window.py#L974-L986
https://github.com/enthought/pyface/blob/2974f580c4c10ec4142641ca67b15ec27593c5ae/pyface/image/image.py#L80

From traits.version we import version

I'd recommend that packages check traits.__version__ instead. (But again, checking the version is a bit of an antipattern - ideally, you check directly for whichever version-sensitive feature you want instead. Why is Pyface using this?)

It's using it in a data_view test for serializing to json:
https://github.com/enthought/pyface/blob/2974f580c4c10ec4142641ca67b15ec27593c5ae/pyface/data_view/tests/test_data_formats.py#L56-L61

From traits.util.resource we import get_path

Not used within Traits, not tested. Possibly redundant with post-Python-3.6 path handling capabilities. What's Pyface using this one for? If it is a cross-ETS need, we could potentially make it available, but it would need tests and a clear use-case.

https://github.com/enthought/pyface/blob/2974f580c4c10ec4142641ca67b15ec27593c5ae/pyface/resource/resource_manager.py#L244-L258
https://github.com/enthought/pyface/blob/2974f580c4c10ec4142641ca67b15ec27593c5ae/pyface/ui/wx/xrc_dialog.py#L57-L65
https://github.com/enthought/pyface/blob/2974f580c4c10ec4142641ca67b15ec27593c5ae/pyface/wx/image.py#L12-L23

From traits.trait_notifiers we import set_ui_handler and ui_handler

These ones may need to be exposed (but not without thought and an understanding of the use-case - if we're going to expose them, we should also take the opportunity to decide whether there's a better API).

These are used in the pyface/ui/{qt4/wx}/init.py files. They check if ui_handler is None and if so, call set_ui_handler(GUI.invoke_later)

[corranwebster]

With some of these, a driver from me is that we don't want multiple, subtly different ways of doing some basic things - the xgetattr/xsetattr thing is one of these, but there are likely others (eg. code for getting "human-friendly" versions of Python names, code for working out where to import a name from, etc.). Maybe they don't belong in Traits, but it's a bad experience for a library if things are not consistent across similar places that they are used.