On 10/8/20 3:04 AM, Kevin Wolf wrote:
The major downside that I saw while reviewing this patch (besides having
extra code just for making the error message of what essentially a
failed assertion nicer) is that we have two names for the same thing, we
have both names in active use in the other methods, and I'll never be
able to remember which of _subp and _popen is the real attribute and
which is the property (or that they are related at all and changing one
will actually change the other, too) without looking it up.
I mean, I guess tools will tell me after getting it wrong, but still...
Properties can make a nice external interface, but I feel using them
internally while you don't avoid accessing the real attribute in methods
other than the property implementation is more confusing than helpful.
Good point. I'll see if I can find a nicer cleanup soon. For now I will
suggest relying on the type checker to spot if we get it wrong.
I do think the little property wrappers are kind of distracting, but
seemed like the quickest means to an end at the time. With type checking
fully in place, refactors can be a little more fearless going forward, I
think.
--js
