Expose xgetattr and xsetattr in traits API

[kitchoi]

TraitsUI uses this xgetattr and xsetattr a lot. A pending PR in Pyface is introducing an example using xgetattr and xsetattr, imported from traits.trait_base (see enthought/pyface#543).

This issue is to propose exposing and therefore maintaining these two functions in traits's public API, making it more obvious to traits developers that these functions are used downstream.

Alternatively, traits can decide not to maintain these functions in the public API. Then pyface and traitsui will need to maintain their own or risk breakages if they are advertising these functions publicly.

Related to #1213

[mdickinson]

This is similar to #1236. And as with #1236, this isn't particularly Traits-related code, and in my opinion, Traits really isn't the place for this sort of general-purpose code. If we expose this in one of the api modules, we're advertising it as something that anyone can use, and in effect promising to maintain it as part of Traits going forward.

I'm strongly opposed to Traits becoming a dumping ground for general utilities purely because it sits at the bottom of the ETS stack. In this case, I think both of (a) having a copy in Pyface which can be adapted to Pyface's own needs, or (b) importing from a non-api module in Traits and accepting the risk that Traits might stop supporting this one day (and that then it'll need to be fixed in Pyface) are better options.

[mdickinson]

The use-case in enthought/pyface#543 is satisfied by a four-line function:

def xgetattr(object, xname):
    for name in xname.split("."):
        object = getattr(object, name, None)
    return object

That seems simple enough that duplicating in Pyface isn't much of a cost. And I'm not sure it even really counts as duplication: it's not clear that the actual need of Traits matches that of Pyface now, or that those needs will continue to match in the future.

[kitchoi]

Thank you. Closing.

[corranwebster]

I disagree about this: it is very annoying to have multiple definitions of the same function scattered around the codebases. Duplicating in Pyface would also imply duplicating in TraitsUI and possibly other places as well.

Maybe traits isn't the place for this sort of core utility routines, but we need somewhere for them.

[mdickinson]

Maybe traits isn't the place for this sort of core utility routines, but we need somewhere for them.

Maybe, maybe not. So far, the things that have come up have been tiny enough that the duplication isn't a real cost; moreover, there's a potential cost to removing the duplication - if the utilities turn out to have slightly different needs, or need to change for different reasons or at different times, then the duplication is better than removal of a fake duplication. DRY is one of the more evil / misapplied rules in software design.

And if we do decide that we need a common place for those utilities, it's still a big leap from there to "they should go in Traits".

Traits is absolutely not the place for utilities that have nothing to do with Traits. We have too many examples of that sort of thing already in Traits, and those examples have already had a cost in maintenance. (For example, the clean_filename function that was used in Envisage, and then turned out to be buggy; the clean_timestamp function really really doesn't belong in Traits, either.)

[mdickinson]

Part of the problem is that as soon as you put something in one of the api modules, you're advertising it for everyone to use.

A halfway option is to treat Pyface and TraitsUI as "friend" packages, that are permitted to import from places in Traits other than the api modules, with full understanding of the increased risk of breakage as a result if Traits needs to move things around, or change their API. But I think that increased risk is acceptable: we control the code for Pyface and TraitsUI, and changes in Traits can be made with full knowledge of the impact of those changes in Pyface and TraitsUI.

In contrast, if we put something in one of the api modules and other projects start using it, it becomes very difficult to change it without knowing what we might be breaking in random projects.

[mdickinson]

It's also worth noting that:

• Traits doesn't even use xgetattr or xsetattr.
• The functions don't have proper docstrings - there's no explanation of the special role that Undefined plays for xgetattr, for example.
• The functions have no unit tests.

So we don't actually need any duplication at all! The functions can be simply removed from Traits, implemented in Pyface, and imported from Pyface by TraitsUI.

[corranwebster]

I guess I don't particularly care about whether these functions are exposed in the .api modules - they aren't core to Pyface or TraitsUI - although it would be nice if they were exposed enough that people aren't tempted to re-implement them themselves. For example, we have an (untested!) topological sort in traits.utils and another one in pyface.tasks (which at least has tests).

But I also think that there may be should be a place for some of these sorts of things where they can happily live in a .api module; and since Traits is where they live right now, this would be the place to discuss it (although we might want to change the name of the issue or open another one). Prime candidates would be the string-mangling routines (camel_case, clean_strings), deprecated, and possibly etsconfig (although that maybe should be its own thing).

Once upon a time, these were in their own library, but when we got rid of the top-level enthought. package many of the got moved into traits for reasons I don't recall.

[corranwebster]

No, traits uses xgetattr in several places.

[mdickinson]

No, traits uses xgetattr in several places.

True. (Four places, to be precise.) I had the 5.2.0 tag checked out, for some reason.

So independently of this issue, xgetattr should really get tests, a decent docstring, and arguably fixes, too: the current version does different things for intermediate lookups than for the final lookup, for no obvious reason: for intermediate lookups, an actual value of None is replaced with default. For the final lookup, a value of None is left as None.

[mdickinson]

although it would be nice if they were exposed enough that people aren't tempted to re-implement them

We're talking about something that's essentially three simple lines:

value = object
for piece in name.split("."):
    value = getattr(value, piece)

At least, the above is the form that's used in Traits. The Pyface usage is different (which is a sign that we probably actually want two different functions):

value = object
for piece in name.split("."):
    value = getattr(value, piece, None)

This isn't code that's easy to get wrong, or encodes some special domain-specific logic, or has ad-hoc decisions or constants that might be changed. It's barely at the level that it's worth abstracting it out into a function in a module, let alone putting it into another package so that it can be imported from that package. This is DRY at its worst: it makes little sense to incur the overhead of implementing, documenting, testing, maintaining and exporting something this simple (as well as importing it where it's needed) versus just writing those three lines of code inline, or in a local function.

[kitchoi]

(Just in case the initial text failed to come across as being impartial.)

For the record, despite being the person opening the issue in the first place, I am not in favour of exposing xgetattr and xsetattr functions in traits public API. In fact I am more in favour of duplicating them in order to reduce dependencies and trivial breakages (evidence). This is a tradeoff between repeating-yourself and managing dependencies. Which way I am more in favour depends on the utility function in question. In both #1236 and this case, the functions are simple enough that I think it is worth repeating.

[mdickinson]

There's one other point here: there's no good reason to expect people on other projects to look in Traits in particular for this sort of thing, given that it's not Traits-related. It's not the natural place to look for random utilities. Users should think of the Traits library as providing Traits and related things; not as providing Traits and various random utilities.

[corranwebster]

Slightly sideways suggestion about naming conventions for ETS projects coming from this discussion:

• if it's in an api.py then it is a formal part of the API for all users and care should be taken with changes
• if it's in a public module with a public name (ie. no leading underscores) then it is "protected": fine for friendly projects to use (eg. other ETS) but might be renamed/moved (again, with some care if it is actually used internally); non-friendly projects should use at their own risk
• if it's private it is private and if you use it you get what you deserve

[kitchoi]

Mostly agreed, but I think we should define "friendly projects". If taken too broadly, it could mean any projects within the enthought organization, and I don't think that was the intention. I think these "friendly projects" should only include projects with a CI job that is tested with traits's master branch. If pyface has such a cron job, any breakage caused by trivial renames and moves can be detected early prior to releases of traits. If pyface ditches its cron job, developers of traits won't be notified, then the "friendliness" would not be practical.

[kitchoi]

For my comment:

Mostly agreed, but I think we should define "friendly projects". If taken ...

I am afraid I have to take back the "mostly agreed" part.

if it's in a public module with a public name (ie. no leading underscores) then it is "protected": fine for friendly projects to use (eg. other ETS) but might be renamed/moved (again, with some care if it is actually used internally); non-friendly projects should use at their own risk

Suppose we took on this "protected" notion, the implication for, e.g. Traits, is that if its "protected" function is used by another ETS package, e.g. TraitsUI, then traits will have to keep backward compatibility of this protected function. The fact that ETS distributions are released at different time means these protected functions are just as rigid to change as if they were public to everybody. In order to be able to change these "protected" functions, all ETS distributions will have to be released at the same time, which defeat the purpose of having separate distribution in the first place.

[mdickinson]

There are really two separate discussions going on in this issue; one about xgetattr and xsetattr specifically, the other about the more general approach we should take towards this kind of thing.

For xgetattr and xsetattr in particular, I'd really like to close and resolve this issue: I think it's clear that Traits is not the right place for these particular utilities, and as the current maintainer of Traits I'm specifically rejecting the proposal to expose xgetattr and xsetattr in any of the Traits api modules, for all the reasons already given.

For the wider discussion, maybe we should open a new issue?

[kitchoi]

For xgetattr and xsetattr in particular, I'd really like to close and resolve this issue: I think it's clear that Traits is not the right place for these particular utilities, and as the current maintainer of Traits I'm specifically rejecting the proposal to expose xgetattr and xsetattr in any of the Traits api modules, for all the reasons already given.

Yes please.

For the wider discussion, maybe we should open a new issue?

Agreed. The ets repo?

[mdickinson]

Agreed. The ets repo?

Yep, that seems like a good place for this discussion.

[corranwebster]

For xgetattr and xsetattr in particular, I'd really like to close and resolve this issue: I think it's clear that Traits is not the right place for these particular utilities, and as the current maintainer of Traits I'm specifically rejecting the proposal to expose xgetattr and xsetattr in any of the Traits api modules, for all the reasons already given.

If not Traits, then we need a place for these utilities, and soon. What I very strongly do not want is subtly different implementations of these sorts of utilities - that's very bad for user expectations.

[mdickinson]

What I very strongly do not want is subtly different implementations of these sorts of utilities

I'd like to focus this issue on xgetattr and xsetattr in particular, and leave the "these sorts of utilities" discussion for the ETS issue.

xgetattr and xsetattr are really simple things; I don't see a downside to repeating the code. Moreover, the actual needs in Traits and the needs in Pyface/TraitsUI don't even exactly match (in terms of behaviour when the value of an attribute is None). In that situation, such duplication is dangerous: a change to the semantics in Traits, aimed at evolving needs of Traits, affects the other packages too.

What I very strongly do not want is subtly different implementations of these sorts of utilities - that's very bad for user expectations.

Maybe better naming / documentation is the answer here? Sometimes different packages have subtly different needs.

[mdickinson]

such duplication is dangerous

Sorry; that came out wrong. What I meant was that the sharing is dangerous, not the duplication.

[mdickinson]

So independently of this issue, xgetattr should really get tests [...]

Opened #1321 for this.

[mdickinson]

Opened enthought/ets#51 for the wider discussion. Closing this issue.

[corranwebster]

I'd argue quite the converse - lack of sharing, in particular with xgetattr was the cause of subtly different behaviour which leads to bugs.

How you do chained lookup is part of the "UI" of traits as a library and ETS more generally, and the strings being parsed by xgetattr are usually supplied by users. You want that consistency (or you end up with situations like with have with import_symbol). You want someone who is writing their own traits who needs to do chained lookup functionality to have something like xgetattr available so their code is guaranteed to work the same way as everyone else's.

So from my point of view:

• I don't care whether or not xgetattr/xsetattr are in an api.py, but it has to be considered OK for other libraries to use them if the consensus is to keep the status quo
• we should not be duplicating them in other libraries (this is a quite strong opinion on my part)
• if the view is that these don't belong in traits, then we need a place for them, and traits should be importing them from there

[mdickinson]

Yes, I could see value in codifying the "right way" to look up an extended trait name on a HasTraits object. That would indeed be a part of the Traits UX/API, and I'd be happy for that to live in Traits and be exposed and documented for others to use.

But xgetattr in its current form is certainly not that right way - it has an unnatural non-obvious design, it has some weird corner case behaviour that's very likely to be a source of bugs in code using it, it's not properly documented, and it has no unit tests. And it's being used for things outside that use-case, too, like extended imports, where the needs are likely to be quite different. That's another common need across ETS: look up this resource from this starting point, but that need is unlikely to be filled by the same function.

Fixing xgetattr is likely to be hard, since we don't know who's using it. But I'd be happy to see new (well-motivated, tested, documented) functionality along those lines, possibly exposed as a method on HasTraits. (I've added a comment to #1321 along these lines.)

[mdickinson]

I am afraid I have to take back the "mostly agreed" part.

Another aspect of this: I think it would be hard in practice to enforce the separation. It seems likely that we'll have people diving into the ETS source, finding a snippet of code that does what they need, and copying it to their own project code, without thinking about the "friendly" versus "everyone else" distinction.

all ETS distributions will have to be released at the same time

I think it's not that bad. We'd just have to be careful to go through the usual deprecation / eventual removal dance - i.e., treat the function in the same way as a public one.

[kitchoi]

It seems likely that we'll have people diving into the ETS source, finding a snippet of code that does what they need, and copying it to their own project code, without thinking about the "friendly" versus "everyone else" distinction.

I agree.

Along this line of "not thinking about the friendly vs everyone else", I don't know if the convention of treating api.py as the public API and all other publicly named modules as "protected" with some definition of affinity is a widely understood convention in the Python community. My guess is that it isn't, and most developers seeing a publicly named module is going to assume backward compatibility support going with version semantics (getting off-topic again, sorry.)

[corranwebster]

That's another common need across ETS: look up this resource from this starting point, but that need is unlikely to be filled by the same function.

This is off-topic, but there is such a function (import_symbol) it has non-trivial semantics and it is implemented at least twice in ETS.

[mdickinson]

there is such a function (import_symbol)

Yes, I'm well aware. :-) The Traits version of that function uses xgetattr. That's what I was referring to when I said "it's being used for things outside that use-case, too, like extended imports".