Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#5193 enhancement closed fixed (fixed)

allYourBase still used in twisted.persisted.styles

Reported by: dustin Owned by: exarkun
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/aybabtu-warnings-5193
(diff, github, buildbot, log)
Author: dustin Launchpad Bug:

Description (last modified by thijs)

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.

Attachments (4)

bug5193-1.patch (2.9 KB) - added by dustin 3 years ago.
5193-1.patch
bug5193-2.patch (64 bytes) - added by dustin 3 years ago.
bug5193-2.patch
bug5193-2.2.patch (2.3 KB) - added by dustin 3 years ago.
bug5193-2.patch
bug5193-3.patch (2.6 KB) - added by dustin 3 years ago.
bug5193-3.patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by 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.

Changed 3 years ago by dustin

5193-1.patch

comment:2 Changed 3 years ago by dustin

  • Keywords review added
  • Owner dustin deleted

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

comment:3 Changed 3 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/aybabtu-warnings-5193

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

comment:4 Changed 3 years ago by exarkun

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

refs #5193

comment:5 Changed 3 years ago by exarkun

  • Author changed from exarkun to dustin
  • Keywords review removed
  • Owner set to dustin

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

Changed 3 years ago by dustin

bug5193-2.patch

comment:6 follow-up: Changed 3 years ago by dustin

  • Keywords review added
  • Owner dustin deleted

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?

comment:7 in reply to: ↑ 6 Changed 3 years ago by thijs

  • Description modified (diff)
  • Keywords review removed
  • Owner set to dustin

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)

comment:8 Changed 3 years ago by dustin

  • Keywords review added
  • Owner dustin deleted

Er, sorry, hg has rotted my brain.

Changed 3 years ago by dustin

bug5193-2.patch

comment:9 Changed 3 years ago by exarkun

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

refs #5193

comment:10 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to 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.

comment:11 Changed 3 years ago by 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.

Changed 3 years ago by dustin

bug5193-3.patch

comment:12 Changed 3 years ago by exarkun

(In [32460]) Fix minor style issues

refs #5193

comment:13 Changed 3 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:14 Changed 3 years ago by 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.

Note: See TracTickets for help on using tickets.