Opened 3 years ago

Closed 3 years ago

#8244 enhancement closed fixed (fixed)

Add a @oldStyle decorator that can selectively upgrade old-style classes to new ones

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/oldstyle-decorator-8244-2
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl


Part of #8243

Change History (9)

comment:1 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/oldstyle-decorator-8244

(In [47049]) Branching to oldstyle-decorator-8244.

comment:2 Changed 3 years ago by hawkowl

Keywords: review added

I am happy with this. Please review.

I guess to fully test this under CI, we will need an (unsupported) builder which runs it with TWISTED_NEWSTYLE=1. In the meantime, you could test it locally using tox.

comment:3 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Awesome, thanks for turning this around so fast hawkowl!

I do have some feedback.

  1. Defining classes and functions inside conditionals is an anti-pattern. Among other things, it breaks IDEs like Jedi and documentation tools like pydoctor. It also provides an opportunity for conflicting signatures, with no way to verify which one is authoritative. Instead, we should have a decorator, like this, so there is one syntactic "place" where the function lives:
    from twisted.python.compat import _PY3
    from twisted.python.util import _replaceIf
    from twisted.internet.defer import passthru
    @_replaceIf(_PY3, passthru)
    @_replaceIf(int(os.environ.get('TWISTED_NEWSTYLE', 0)) == 0,
    def _oldStyle(cls):
        blah blah describe how _oldStyle works
        # py2 impl
    You could skip this step entirely though, if you want; the reason to do the detection at import time is to avoid repeatedly running the test at runtime, but _oldStyle could just be implemented entirely at "run time" because it's a decorator that is only ever run at import time anyway. (You could still decrease the number of times the test is performed, of course, so possibly still worthwhile.)
  2. In the case of the test cases, we already have test-skipping (the "skip" attribute); use it, don't define the test conditionally.
  3. What we want here is "new style if TWISTED_NEWSTYLE, otherwise old-style". What's implemented here, however, is "hybrid if TWISTED_NEWSTYLE, otherwise old-style"; the classic class still appears in the inheritance hierarchy. In order to eliminate ClassType from the hierarchy entirely, you will have to manually construct a new type object by calling type() directly with the __dict__ and __bases__ from the passed class object.

Please address these issues to your satisfaction and resubmit for review.

Additionally, you might want to format your docstrings with for easier formatting. If you use vim (do you use vim?) a snippet like this might get you started:

function! WrapDocstring()
    let l:where = getpos(".")
    let l:indent = indent(".")
    let l:there = getpos(".")
    call setpos(".", l:where)
    exe "?\"\"\"?1,/\"\"\"/-1!python path/to/python-docstring-mode/ --linewise --indent=" . l:indent
nnoremap gqaa :call WrapDocstring()<CR>

I should probably put that in version control somewhere.

comment:4 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi glyph, thanks for the review.

  1. Done. This is a good idea (the decorator), and bits of compat could be reformatted to use it and make it much nicer!
  1. Done. I just didn't want to try and walk every module, but it looks like that takes like half a second, so, it's fine. It's only temporary, anyway.
  1. Fixed. It should work fine, I think.

Builds spun, all green, please review.

comment:5 Changed 3 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Thanks for addressing everything. Much more minor feedback this time.

  1. passthru is defined already, in twisted.internet.defer. It has no docstring there, but it's public API. Maybe you could delete some lines out of there and improve the documentation at the same time by putting passthru somewhere else and doing an __all__ move into defer. On second thought this makes more sense as a follow-up ticket.
  2. The expression int(os.environ.get('TWISTED_NEWSTYLE', 0)) == 0 is a bit too fragile for my liking. This provides a new way for the user to accidentally screw up their environment in a way which causes Twisted to inscrutably refuse to import. I am especially worried about the common shell invocation TWISTED_NEWSTYLE= twistd foo ... which doesn't actually un-set that environment variable, it just sets it to the empty string. And as you know, int("") is ValueError: invalid literal for int() with base 10: ''. A better expression would be bool(os.environ.get('TWISTED_NEWSTYLE', "")).
  3. The docstring for _oldStyle uses quotes to offset TWISTED_NEWSTYLE as an identifier, but we have a syntax for that in epydoc, C{}. It would also read slightly more nicely if it said "if the environment variable "TWISTED_NEWSTYLE" is empty or not set", leading with "environment variable" rather than making the reader wonder what kind of thing it is initially since it's not an L{}.
  4. test_nooldstyle repeats the os.environ.get idiom; it should be a function that just gets imported and called. As a bonus this will make it easier to add error-reporting and warnings later. (Ideally I'd like the interface to this to be something like what pip does, where no and false and off are all "no" values, and anything not in the expected set gives you a warning and explains you should be explicit; doing that takes a couple of lines of code that won't fit in a snippet like this though.)
  5. We should talk about how you can get docstring-mode installed ;-)
  6. On the error reporting on _replaceIf, it would be nice if it repr'd the actual value. It's always a little frustrating to be reading a traceback from something and not know what was provided. expected bool got {} is really all you need. My future stack overflow reputation thanks you.
  7. The test cases in test_nooldstyle are ... not quite what I expected. Maybe better than what I expected, though, in certain ways! A few things:
    1. Try not to put any actual code expressions at the top level scope. Put the code in a function, then call it immediately afterwards. Among other things, this prevents unintentional namespace pollution. You cleaned up Test (presumably to prevent Test from getting test-discovered), but you didn't clean up acceptableName or is_ignored or ignored or x.
    2. Rather than generating a zillion test classes, you could instead generate a zillion test methods, which would have shorter identifiers.
    3. I had intended for this test to run normally, against a whitelist, to prevent us from adding any new old-style classes. I would still like this - it will make it much easier for contributors to immediately see if they've screwed this up locally before submitting for review and relying on a special builder. Upon reflection I suppose the decorator itself is sufficient (_oldStyle should say in its docstring that it shouldn't be added to anything new, and we should mention this on the mailing list, maybe add it to the reviewer checklist). In any case, since this is a huge but mechanical diff, let's just make this a task for a follow-up.

Aside from making the environment variable more permissive, I think that this is all optional; none of it would appreciably change the implementation. Please address to your satisfaction and land.

comment:6 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [47119]) Merge oldstyle-decorator-8244: Add a decorator which optionally updates old-style classes to new-ones

Author: hawkowl Reviewer: glyph Fixes: #8244

comment:7 Changed 3 years ago by hawkowl

Resolution: fixed
Status: closedreopened

(In [47120]) Revert "Merge oldstyle-decorator-8244: Add a decorator which optionally updates old-style classes to new-ones"

Whoops, some pyflakes errors fell through the cracks!

Reopens: #8244

comment:8 Changed 3 years ago by hawkowl

Branch: branches/oldstyle-decorator-8244branches/oldstyle-decorator-8244-2

(In [47121]) Branching to oldstyle-decorator-8244-2.

comment:9 Changed 3 years ago by hawkowl

Resolution: fixed
Status: reopenedclosed

(In [47126]) Merge oldstyle-decorator-8244-2: Add a decorator which optionally updates old-style classes to new-ones

Author: hawkowl Reviewer: glyph Fixes: #8244

Note: See TracTickets for help on using tickets.