Skip to content

Print sources for fget/fset/fdel property methods #4827

Merged
merged 3 commits into from Jun 18, 2014

6 participants

@immerrr
immerrr commented Jan 18, 2014

This is a work-in-progress fixing issue #1555 which has been bugging me for quite a while. Here's what it is capable of now:

In [1]: def foo(x): return id(x)

In [2]: p = property(foo, lambda x,val: None, id)

In [3]: p?
Type:       property
String Form:<property object at 0x2c52c00>
File:       /home/immerrr/sources/<ipython-input-1-8d2e34e1a2b1>
Docstring:  <no docstring>

In [4]: p??
Type:       property
String Form:<property object at 0x2c52c00>
File:       /home/immerrr/sources/<ipython-input-1-8d2e34e1a2b1>
Getter:     def foo(x): return id(x)

Setter:     p = property(foo, lambda x,val: None, id)

Deleter:    <function id>

In [5]: class Foo(object):
   ...:     @property
   ...:     def bar(self):
   ...:         return None
   ...:     

In [6]: f = Foo()

In [7]: f.bar?
Type:       property
String Form:<property object at 0x2f84aa0>
File:       /home/immerrr/sources/<ipython-input-5-64d3245b5460>
Docstring:  <no docstring>

In [8]: f.bar??
Type:       property
String Form:<property object at 0x2f84aa0>
File:       /home/immerrr/sources/<ipython-input-5-64d3245b5460>
Getter:
    @property
    def bar(self):
        return None

Admittedly, this is not perfect, so ideas about what's missing are welcome. What I think is missing:

  • tests (didn't look how testing is done in IPython, nor do I have an idea how to test this properly)
  • get rid of newlines that still appear after one-liner source snippets
  • stop invoking property getter when querying as it may cause unintended side-effects and/or raise an error rendering the property inaccessible
@ellisonbg ellisonbg was assigned Jan 23, 2014
@takluyver
IPython member

This changes the info dict so the 'source' field can be a list of 2-tuples. I think the info dict also gets sent to frontends on an object_info_request, so changes to its structure might break frontends. We're also trying to take it in the direction of being more language agnostic, which might need some more thought. Do you want to join our next online dev meeting (Thursday, 10am PST) to discuss this more?

@immerrr
immerrr commented Jan 25, 2014

I've checked that hack with "regular" frontends and it seemed to work, but I agree that interface concerns are perfectly reasonable.

In that case it may be structurally cleaner to embrace the fact that there can be multiple "kinds" of sources for objects and add those additional "kinds" of sources as separate fields into "info" dictionary, e.g. descriptor protocol sources (properties being a special case of that), places where constants of built-in types are set and so on. I thought that these things should be figured out by now in libraries like rope or jedi (at least to some extent), so relying on code introspection libraries to do actual code introspection may be a win-win. I'll think in that direction.

@immerrr
immerrr commented Jan 25, 2014

As for the meeting, yeah, sure, why not. I'm not sure if I qualify as an "IPython dev" as this is probably the first time I got my hands dirty with its internals, but yeah. As a matter of fact I've had this idea of implementing something IPython-ish* for Lua someday, but the language-agnostic direction you've brought up may make this someday a lot closer.

  • disclaimer: I'm maintaining lua-mode for Emacs in spare time and the original intention was to make something like SLIME, but I guess not all languages as introspectable as Common Lisp.
@Carreau
IPython member
Carreau commented Jan 25, 2014

That one of the reason we like to have people from non-python world, to try to abstract what is needed for all programming languages.

As far as your concern to qualify as an "IPython dev", you submitted a PR, which I suppose is enough. IPython don't have users, only dev :-). After you have the "core dev" who have commit right, but this is just when you start contributing regularly and we just can't keep up with the review, so we give you commit right :-)

As far as the Meeting they are live (and archived) on youtube, so yo can look at them whenever you want.
Just tell us you like to hangout live, and we'll send you an invitation by mail when the hangout start.
We filter people on live hangout because google have difficulties handeling more than 10.

@immerrr
immerrr commented Jan 29, 2014

@Carreau, Ok, let's try then, where do I sign? :)

@Carreau
IPython member
Carreau commented Jan 29, 2014

@Carreau, Ok, let's try then, where do I sign? :)

Meetings are live on youtube (google hangout) every thurday 10am PST, usually with a chat open here and agenda just request an invite (ie we need your email) if you want to join on a specific session and @minrk will send you a google hangout invite a few minutes before we start.

@immerrr
immerrr commented Jan 30, 2014

Ok, not sure if I'll be back home in time, but I'll request an invite if I do. Thanks.

@Carreau
IPython member
Carreau commented Jan 30, 2014

Ok, not sure if I'll be back home in time, but I'll request an invite if I do. Thanks.

It's every week, no hurry. But today /tomorow, we might speek of completer already.

@immerrr
immerrr commented Feb 11, 2014

Ok, I have:

  • rebased this onto master
  • rewritten it to put all sources into info['source'] field
  • patched ofind to avoid invoking the last attribute lookup if it's a property
  • added tests

It now looks something like that:

In [6]: class A(object):
    id = property(id)
    @id.setter
    def id(self, val):
        raise NotImplementedError("You cannot set object's id")

In [10]: a.id??
Type:       property
String Form:<property object at 0x1b1e0a8>
Source:
# a.id.fget
a.id.fget = <function id>

# a.id.fset
@id.setter
def id(self, val):
    raise NotImplementedError("You cannot set object's id")

Is there a possibility that this gets merged before 3.0? :)

@takluyver
IPython member

Yes, I imagine this will be merged before 3.0, but that's still some time away - we're aiming for 2.0 in the next few weeks.

@ellisonbg
IPython member

Needs rebase.

@immerrr
immerrr commented Mar 4, 2014

Done.

@immerrr
immerrr commented Mar 8, 2014

Rebased once more and fixed tests on Py3.

@minrk
IPython member
minrk commented May 15, 2014

Sorry for the long silence. Now that the msg spec changes are merged, the object_info_reply implications of this PR are no longer an issue. I don't see anything more to do here. Is this okay to merge, @takluyver?

@jdfreder
IPython member
jdfreder commented Jun 3, 2014

bump just in case @takluyver hasn't seen this since he was away for a while there.

@takluyver takluyver commented on the diff Jun 4, 2014
IPython/core/interactiveshell.py
try:
parent = obj
- obj = getattr(obj,part)
+ # The last part is looked up in a special way to avoid
+ # descriptor invocation as it may raise or have side
+ # effects.
+ if idx == len(oname_rest) - 1:
+ obj = self._getattr_property(obj, part)
+ else:
+ obj = getattr(obj, part)
@takluyver
IPython member
takluyver added a note Jun 4, 2014

Using a loop and checking if we're on the last iteration seems icky. Why not split oname up into oname_head, oname_rest and oname_tail, leave the loop as it is, and do the last bit outside the loop:

obj = self._getattr_property(obj, oname_tail)
@immerrr
immerrr added a note Jun 15, 2014

It appears that the whole loop must be duplicated outside (carpet catch, parent preservation) and for: ... else: ... block can no longer be used to check if the lookup succeeded, so an extra variable must be introduced. Does it look icky enough for that or did you have anything else in mind?

@takluyver
IPython member
takluyver added a note Jun 16, 2014

I guess you're right, the approach you've got is better than the alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@takluyver takluyver and 1 other commented on an outdated diff Jun 4, 2014
IPython/core/oinspect.py
@@ -705,11 +708,33 @@ def info(self, obj, oname='', formatter=None, info=None, detail_level=0):
linecache.checkcache()
source = None
try:
- try:
- source = getsource(obj, binary_file)
- except TypeError:
- if hasattr(obj, '__class__'):
- source = getsource(obj.__class__, binary_file)
+ if isinstance(obj, property):
+ sources = []
@takluyver
IPython member
takluyver added a note Jun 4, 2014

This whole section is only active when we do A.b?? to see the source. Shouldn't we be looking up property docstrings when we do single-question-mark A.b? introspection? At least for the getter, as the most common use of properties is getter-only.

Also, a comment explaining what's going on would be good here - the whole of oinspect can get pretty hard to follow.

@immerrr
immerrr added a note Jun 15, 2014

re: single-? — I'm pretty sure I had given this a thought when implementing, but can't remember what was the rationale. Will fix that.

re: comment — ok.

@immerrr
immerrr added a note Jun 16, 2014

re: single-? — Apparently, inspect.getdoc(property(...)) already looks up proper docstring and so, as long as property objects are found via ofind, it should work. It doesn't currently work for other descriptors, like slot attributes, though:

In [66]: ar = np.array([1])

In [67]: ar.shape?
Type:        tuple
String form: (1,)
Length:      1
Docstring:
tuple() -> empty tuple
tuple(iterable) -> tuple initialized from iterable's items

If the argument is a tuple, the return value is the same object.

In [68]: np.ndarray.shape?
Type:        getset_descriptor
String form: <attribute 'shape' of 'numpy.ndarray' objects>
Docstring:
Tuple of array dimensions.

Notes
-----
May be used to "reshape" the array, as long as this would not
require a change in the total number of elements

Examples
--------
>>> x = np.array([1, 2, 3, 4])
>>> x.shape
(4,)
>>> y = np.zeros((2, 3, 4))
>>> y.shape
(2, 3, 4)
>>> y.shape = (3, 8)
>>> y
array([[ 0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.],
       [ 0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.],
       [ 0.,  0.,  0.,  0.,  0.,  0.,  0.,  0.]])
>>> y.shape = (3, 6)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: total size of new array must be unchanged

@takluyver
IPython member
takluyver added a note Jun 16, 2014

OK, it sounds like that's good enough for now. If you want to drop those couple of extra comments in here, I think this is ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@takluyver takluyver and 1 other commented on an outdated diff Jun 4, 2014
IPython/core/oinspect.py
+ sources.append(cast_unicode(
+ ''.join(('# ', oname_prefix, attrname)),
+ encoding=encoding))
+ if inspect.isfunction(fn):
+ sources.append(dedent(getsource(fn)))
+ else:
+ # Default str/repr only prints function name,
+ # pretty.pretty prints module name too.
+ sources.append(cast_unicode(
+ '%s%s = %s\n' % (
+ oname_prefix, attrname, pretty(fn)),
+ encoding=encoding))
+ if sources:
+ source = '\n'.join(sources)
+ else:
+ try:
@takluyver
IPython member
takluyver added a note Jun 4, 2014

Likewise a comment here pointing out that this section gets source for objects other than properties would be useful.

@immerrr
immerrr added a note Jun 15, 2014

ok, I'll refactor/comment this bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@takluyver
IPython member

Thanks for your patience, @immerrr - this basically looks good, I just spotted a couple of things in review. The most important one is that I think we should show some property docstring details with the single-question-mark introspection that doesn't show source.

@immerrr
immerrr commented Jun 18, 2014

I've rebased/refactored the code a bit.

Now it's tested that info contains property docstrings as expected and the property-specific source lookup code is put into oinspect.getsource so that %psource command could work the same way.

I couldn't resist a bit of refactoring of getsource on the way, though:

  • binary_file parameter was passed in only to short-circuit and exit from it, so I've moved it outside
  • I've also added an optional oname parameter to make property formatter work as earlier

Two further considerations.

  • Now it seems that binary_file flag in info method can be removed, too: if obj comes from a binary file, getsource should gracefully return None (or at least it could be made to do so).

  • Also, while investigating the getset_descriptor objects in numpy, I've run into a funny file: numpy/numpy@numpy/add_newdocs.py which kind of shows that there exists at least one sane use case when docstrings may be attached to a live object but not present in its immediate source. Maybe the logic that drops docstrings if the source is shown should do some duplication detection to handle that.

@takluyver takluyver commented on the diff Jun 18, 2014
IPython/core/oinspect.py
@@ -150,33 +153,68 @@ def getdoc(obj):
return None
-def getsource(obj,is_binary=False):
+def getsource(obj, oname=''):
@takluyver
IPython member
takluyver added a note Jun 18, 2014

This is a backwards incompatible change, so please write a note in docs/source/whatsnew/pr/incompat-oinspect-getsource.rst (the name doesn't matter, so long as it starts with incompat-).

@immerrr
immerrr added a note Jun 18, 2014

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@takluyver takluyver and 1 other commented on an outdated diff Jun 18, 2014
IPython/core/oinspect.py
try:
- try:
- source = getsource(obj, binary_file)
- except TypeError:
- if hasattr(obj, '__class__'):
- source = getsource(obj.__class__, binary_file)
- if source is not None:
- out['source'] = source.rstrip()
+ if isinstance(obj, property) or not binary_file:
+ out['source'] = getsource(obj, oname).rstrip()
@takluyver
IPython member
takluyver added a note Jun 18, 2014

getsource can still return None, in which case rstrip() will fail.

@immerrr
immerrr added a note Jun 18, 2014

try-catch around it wouldn't allow that to do any harm, but still a good catch, will fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@immerrr
immerrr commented Jun 18, 2014

I've revisited property lookup a bit after going through Python reference: it turns out properties (class-level data descriptors) take precedence over instance-level attributes, so property-aware look up doesn't have to peek into object's dictionary if its class is known to have a property by the sought name.

@takluyver takluyver and 1 other commented on an outdated diff Jun 18, 2014
docs/source/whatsnew/pr/incompat-oinspect-getsource.rst
@@ -0,0 +1,4 @@
+* :func:`core.oinspect.getsource` call specification has changed:
@takluyver
IPython member
takluyver added a note Jun 18, 2014

I think this needs the fully qualified name, i.e. starting with IPython., for Sphinx crosslinking to work. I may be wrong about that, though.

@immerrr
immerrr added a note Jun 18, 2014

Yup, you're right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@takluyver
IPython member

Thanks @immerrr , I think this is ready to go now. Are you happy with it?

@takluyver
IPython member

(The Travis failure is an unrelated thing in a JS test)

immerrr added some commits Jan 18, 2014
@immerrr immerrr Print sources for property fget/fset/fdel methods
This commit incorporates a backward-incompatible change:
`core.oinspect.getsource` will no longer accept `is_binary` kwarg.
783754c
@immerrr immerrr core.interactiveshell.ofind: don't evaluate property.fget (it may raise) 02f9be0
@immerrr immerrr Use inspect.getdoc when populating _builtin_type_docstrings
IPython.core.oinspect.getdoc uses inspect.getdoc which
strips some whitespace internally and because of that

    inspect.getdoc(property) != property.__doc__
1fd9014
@immerrr
immerrr commented Jun 18, 2014

Are you happy with it?

Yeah, sure.

@takluyver
IPython member

Great, merging once we get the green light from Travis.

@takluyver takluyver merged commit 620860b into ipython:master Jun 18, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@immerrr
immerrr commented Jun 19, 2014

Awesome, thanks for your support.

@immerrr immerrr deleted the immerrr:print-property-getter-setter-deleter-sources branch Jun 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.