py.code.Code "path" attribute not always a py.path.local object

[pytestbot]

• Bitbucket: https://bitbucket.org/pytest-dev/py/issue/71
• Originally reported by: @nicoddemus
• Originally created at: 2015-09-30T03:16:42.939

Sometimes the path attribute of a py.code.Code object is not a py.local.path object but a plain string, usually due to a traceback entry not being located in a real file but as a result of an exec statement.

import sys
def test_entry_not_path():
    try:
        ns = {}
        exec('def foo(): raise ValueError', ns)
        ns['foo']()
    except ValueError:
        _, _, tb = sys.exc_info()
    tb = py.code.Traceback(tb)
    assert isinstance(tb[-1].path, py.path.local)

This caused a regression in pytest, as seen in #995.

[pytestbot]

Original comment by @nicoddemus

OK, thanks.

[pytestbot]

Original comment by @hpk42

so i guess it's ok to always just return py.path.local(co_filename) without any checks -- but we should see if any tests fail. The pytest pytest-dev/pytest#995 should be solved without relying on this IMO.

[pytestbot]

Original comment by @hpk42

I now think that the code.path API is not such a good idea to begin with. if co_filename contains "<something>" (a relative path if you will) then py.path.local(...) will make it into an absolute path which is strange. Returning None or a string does not really make it better. Maybe the original pytest issue should be solved differently (and we can still simplify the code.path impl, agreed). i'll comment over there :)

[pytestbot]

Original comment by @nicoddemus

The question is what be the benefit of exposing this to the user? It goes against what the docs say, Python itself doesn't do this distinction (after all it is always stored in co_filename), introduces the possibility of subtle bugs ("in rare situations this may be None")... I might be missing something, but I don't see any benefit of having path return anything other than a py.path.local object.

[pytestbot]

Original comment by @RonnyPfannschmidt

there is a important detail we must take into a account, a localpath represents an absolute path and the given location for the code is a placeholder that's specifically not a path

[pytestbot]

Original comment by @nicoddemus

I really disagree... I don't think returning None will have any benefits at all, only downsides. Returning py.path.local streamlines the API and avoids errors. For example returning None would still trigger a bug in pytest (NoneType has no attribute relto).

[pytestbot]

Original comment by @RonnyPfannschmidt

i think we need to return None there, when its not a path

[pytestbot]

Original comment by @RonnyPfannschmidt

path is always absolute, thus not suitable for markers like

[pytestbot]

Original comment by @nicoddemus

IMO path should always return a py.path.local object, otherwise now every client code must explicitly check if p.path is a string or risk running into a bug, which is very error prone (as the pytest bug demonstrates) and clunky API overall. The fact that Python uses the same co_filename field for this situation just makes a stronger case for following suit and not try to make it a special case at all.

Other situations where this filename might not be valid is when distributing pyc files: in this case the co_filename points to a path in the machine that generated it, not in the machine running it, so the user must p.check() anyway if it wants to do something with the file. In the current situation the user has to also check if the entry.path is a str or py.path.local object, adding an unnecessary burden to the user. Consider:

def use_entry(entry):
    if isinstance(entry.path, str) and os.path.isfile(entry.path) or entry.check():
        # do something with filename

Most users would now arguably just write:

def use_entry(entry):
    path = py.path.local(entry.path)
    if entry.check():
        # do something with the file at entry.path

So it makes sense to always return a py.path.local object from TracebackEntry.path in the first place. The patch is trivial (also note that the docstring already accounts for the situation where the path might not point to a real file):

    @property
    def path(self):
        """ return a path object pointing to source code (note that it
        might not point to an actually existing file). """
        p = py.path.local(self.raw.co_filename)
+       return p
-        # maybe don't try this checking
-        if not p.check():
-            # XXX maybe try harder like the weird logic
-            # in the standard lib [linecache.updatecache] does?
-           p = self.raw.co_filename
-        return p

(This also fixes the py.test bug)

In summary, I see no downsides in making TracebackEntry.path always return a py.path.local object: simpler and less error-prone API. Keep the API returning both a py.path.local object and str brings only problems and no benefit.

[pytestbot]

Original comment by @hpk42

Should we maybe jsut document that code.path can be a string in case there is no underlying path due to the code being dynamically generated?

[RonnyPfannschmidt]

closing as py.code is no longer actually maintained