allYourBase still used in twisted.persisted.styles

[dustin]

I see

  /home/dustin/code/buildbot/t/buildbot/sand27/lib/python2.7/site-packages/twisted/persisted/styles.py:160: DeprecationWarning: twisted.python.reflect.allYourBase was deprecated in Twisted 11.0.0; please use inspect.getmro instead

in my logs quite a lot. Obviously it's harmless, but should be fixed.

[dustin]

As #4928 mentions, this is one of the few places that the second argument to allYourBase is used, and as such getmro isn't a drop-in replacement for it. #3843 recommends deprecating styles.Versioned, but that hasn't happened yet and anyway, it doesn't make sense for one part of twisted to use a function deprecated in another part of Twisted.

[dustin]

5193-1.patch

[dustin]

The included tests pass with the old (allYourBase) implementation of _aybabtu, as well.

[exarkun]

(In [32444]) Branching to 'aybabtu-warnings-5193'

[exarkun]

(In [32445]) Apply bug5193-1.patch

refs #5193

[exarkun]

Thanks. Some simple points:

1. There's extra space in the list definition in _aybabtu, should be [c, Versioned]
2. The new test method should be test_aybabtu rather than testAybabtu
3. The new test method's docstring is pretty weak. Don't write docstrings that say "Test foo". It's a test method, what else would it be doing? :) Write docstrings that describe desired behavior, to give the reader of the test method implementation some context.
4. This test could really be 4 different tests, probably.
5. AybabtuTests could use a docstring too (this is a more appropriate place to write "Test foo", though some extra description of what foo does in general is also nice).

[dustin]

bug5193-2.patch

[dustin]

This (bug5193-2.patch) should fix all of the above points. The docstrings feel a little bit like "x++; // add one to x" to me, but I think they are what you want?

[thijs]

Replying to dustin:

This (bug5193-2.patch) should fix all of the above points. The docstrings feel a little bit like "x++; // add one to x" to me, but I think they are what you want?

Thanks. The patch only contains the following though:

# On branch bug5193
nothing to commit (working directory clean)

[dustin]

Er, sorry, hg has rotted my brain.

[dustin]

bug5193-2.patch

[exarkun]

(In [32459]) Apply bug5193-2.2.patch

refs #5193

[exarkun]

Thanks! Some style points:

Docstrings should be formatted like:

def foo():
    """
    bar
    """

Test methods should be like:

def test_fooBarBaz(self):
    ...

Since there is a built in type set, the test method docstrings should probably talk about list instead, as that is the actual return type.

These are minor points, I'll make the changes and merge the branch. Thanks again.

[dustin]

Actually, that raises a good point - the order of the results is not guaranteed by _ababtu, and the tests should reflect that -- probably by testing assertEqual(sorted(...), sorted([expected]). I'll make another patch - one moment.

[dustin]

bug5193-3.patch

[exarkun]

(In [32460]) Fix minor style issues

refs #5193

[exarkun]

(In [32461]) Merge aybabtu-warnings-5193

Author: dustin Reviewer: exarkun Fixes: #5193

Remove the use of the deprecated twisted.python.reflect.allYourBase from twisted.persisted.styles.

[exarkun]

Actually, that raises a good point - the order of the results is not guaranteed by _ababtu

Clearly trac is not good for realtime interactions. :/ However, the ordering isn't non-deterministic, so I don't have a problem with the tests asserting a particular order.