Opened 22 months ago

Last modified 20 months ago

#6066 enhancement new

Create utility function for getting name of a function for use by __str__

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/functionname-6066
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

Various places in Twisted (LoopingCall, DelayedCall) have logic for getting a string representation of functions. This logic should be unified in a single utility function in twisted.python.reflect.

Attachments (2)

6066.diff (3.5 KB) - added by borko 20 months ago.
diff v1
6066_v2.diff (5.6 KB) - added by borko 20 months ago.
diff 2

Download all attachments as: .zip

Change History (14)

comment:1 Changed 22 months ago by itamar

  • Keywords easy added
  • Milestone Python 3.3 Minimal deleted

comment:2 Changed 20 months ago by borko

  • Owner set to borko
  • Status changed from new to assigned

Changed 20 months ago by borko

diff v1

comment:3 Changed 20 months ago by borko

  • Keywords review added; easy removed

comment:4 Changed 20 months ago by itamar

  • Keywords review removed
  • Status changed from assigned to new

Thanks for submitting this! It's very nice to get patches with tests.

  1. I will note that we're trying to get rid of _reflectpy3 (see #6183), but until then if you've added code to _reflectpy3 you should make sure it's publicly available from twisted.python.reflect (i.e. appears in __all__ in the latter module).
  2. The docstring for getFunctionName indicates it will return None, which is not the case.
  3. I would call the argument to getFunctionName something like function; Python doesn't have pointers.
  4. You need a test for the safe_repr() case, if I'm not mistaken.
  5. Your tests talk about the "second argument", but there is only one argument.

comment:5 Changed 20 months ago by itamar

Please fix the above and resubmit.

Changed 20 months ago by borko

diff 2

comment:6 Changed 20 months ago by borko

  • Keywords review added

comment:7 Changed 20 months ago by borko

Thx for fast answer!

  1. Do we want to dispose of the reflectpy3 file by slowly porting all the code to reflectpy ? Or move it to other modules ?

comment:8 Changed 20 months ago by itamar

  • Owner borko deleted

We'll restore the code to its original home in reflect.py, using a slightly tricky procedure since we want to preserve version control history and pretend _reflectpy3.py never happened.

comment:9 Changed 20 months ago by borko

second patch created. waiting for acceptance.

comment:10 Changed 20 months ago by itamarst

  • Author set to itamarst
  • Branch set to branches/functionname-6066

(In [36575]) Branching to 'functionname-6066'

comment:11 Changed 20 months ago by itamar

  • Keywords review removed
  • Owner set to itamar

Looks good, just needs slightly better descriptions and a news file; I will do that and merge.

comment:12 Changed 20 months ago by itamarst

(In [36601]) Apply patch, with some minor fixes. Refs #6066

Note: See TracTickets for help on using tickets.