python / cpython

The Python programming language
https://www.python.org
Other
62.84k stars 30.1k forks source link

Direct sub-classing of pathlib.Path #68320

Closed 114124f8-a54e-4a8a-a5ba-52165f132225 closed 1 year ago

114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago
BPO 24132
Nosy @pfmoore, @pitrou, @keithy, @qb-cea, @miss-islington, @FFY00, @barneygale, @Xtrem532, @nyuszika7h, @kfollstad
PRs
  • python/cpython#6248
  • python/cpython#25240
  • python/cpython#25271
  • python/cpython#25701
  • python/cpython#26141
  • python/cpython#26438
  • python/cpython#26708
  • python/cpython#26906
  • python/cpython#31085
  • python/cpython#31691
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'library', '3.11'] title = 'Direct sub-classing of pathlib.Path' updated_at = user = 'https://bugs.python.org/projetmbc' ``` bugs.python.org fields: ```python activity = actor = 'barneygale' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'projetmbc' dependencies = [] files = [] hgrepos = [] issue_num = 24132 keywords = ['patch'] message_count = 28.0 messages = ['242643', '242651', '242656', '242657', '242689', '242696', '242699', '242700', '242701', '242702', '242703', '242705', '246007', '305799', '305811', '305827', '305830', '305918', '310634', '314561', '314582', '314625', '365277', '381321', '392695', '401946', '412354', '414821'] nosy_count = 15.0 nosy_names = ['paul.moore', 'pitrou', 'bronger', 'elguavas', 'piotr.dobrogost', 'Kevin.Norris', 'projetmbc', 'keithy', 'qb-cea', 'miss-islington', 'FFY00', 'barneygale', 'Xtrem532', 'nyuszika7h', 'kfollstad'] pr_nums = ['6248', '25240', '25271', '25701', '26141', '26438', '26708', '26906', '31085', '31691'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue24132' versions = ['Python 3.11'] ```

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago

    Hello.

    I have noticed a problem with the following code.

    from pathlib import Path
    
    class PPath(Path):
        def __init__(self, *args, **kwargs):
            super().__init__(*args, **kwargs)
    
    test = PPath("dir", "test.txt")

    This gives the following error message.

    
    Traceback (most recent call last):
      File "/Users/projetmbc/test.py", line 14, in <module>
        test = PPath("dir", "test.txt")
      File "/anaconda/lib/python3.4/pathlib.py", line 907, in __new__
        self = cls._from_parts(args, init=False)
      File "/anaconda/lib/python3.4/pathlib.py", line 589, in _from_parts
        drv, root, parts = self._parse_args(args)
      File "/anaconda/lib/python3.4/pathlib.py", line 582, in _parse_args
        return cls._flavour.parse_parts(parts)
    AttributeError: type object 'PPath' has no attribute '_flavour'

    This breaks the sub-classing from Python point of view.

    There is an ugly hack to sub-class Path but it's a bit unpythonic.

    pfmoore commented 9 years ago

    One issue with your code - what would you expect str(test) to produce? "dir/test.txt" or "dir\test.txt"? That's the point of the "flavour" - is it a Windows path or a Unix path?

    Agreed that an easier method of creating Path subclasses that handle this type of thing would be useful, but any solution needs to make sure that developers don't overlook the Windows vs Unix implications.

    Can you give an actual use case (as opposed to the toy example)?

    pitrou commented 9 years ago

    The Path classes were not designed to be subclassable by the user. I'm not against making subclassing easier, but someone will have to propose a viable approach for that.

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago

    Hello.

    I will give a real example in 5 hours after my job. I will try tomorrow a solution to ease the subclassing using another dedicazted class PathPlus, sorry for the name. The idea would be to use this new class for customization, and also to define WindowsPath and PosixPath sub-classing this new class. By default PathPlus would be an empty class. I do not know if this works well. Maybe my idea is a bad one.

    *Christophe BAL* *Enseignant de mathématiques en Lycée **et développeur Python amateur* *--- *French math teacher in a "Lycée" **and **Python **amateur developer\

    2015-05-06 13:05 GMT+02:00 Antoine Pitrou \report@bugs.python.org\:

    Antoine Pitrou added the comment:

    The Path classes were not designed to be subclassable by the user. I'm not against making subclassing easier, but someone will have to propose a viable approach for that.

    ---------- versions: +Python 3.5 -Python 3.4


    Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue24132\


    114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago

    Here are for example two extra methods that I have implemented.

    def __sub__(cls, path):
        """
    This magic method allows to use ``onepath - anotherpath`` instead of the
    long
    version ``onepath.relative_to(anotherpath)`` given by ``pathlib.Path``.
        """
        return cls.relative_to(path)
    
    def _ppath_common_with(cls, paths):
        """
    This method returns the path of the smaller common "folder" of the current
    path
    and at least one paths.

    python:: from mistool import os_use

        path   = os_use.PPath("/Users/projects/source/doc")
        path_1 = os_use.PPath("/Users/projects/README")
        path_2 = os_use.PPath("/Users/projects/source/misTool/os_use.py")
    
        print(path.common_with((path_1, path_2)))
        """
        if not isinstance(paths, (list, tuple)):
            paths = [paths]
    
        commonparts = list(cls.parts)
    
        for onepath in paths:
            i = 0
    
            for common, actual in zip(commonparts, onepath.parts):
                if common == actual:
                    i += 1
                else:
                    break
    
            commonparts = commonparts[:i]
    
            if not commonparts:
                break
    
        commonpath = pathlib.Path("")
    
        for part in commonparts:
            commonpath /= part
    
        return commonpath

    *Christophe BAL* *Enseignant de mathématiques en Lycée **et développeur Python amateur* *--- *French math teacher in a "Lycée" **and **Python **amateur developer\

    2015-05-06 14:13 GMT+02:00 Christophe BAL \report@bugs.python.org\:

    Christophe BAL added the comment:

    Hello.

    I will give a real example in 5 hours after my job. I will try tomorrow a solution to ease the subclassing using another dedicazted class PathPlus, sorry for the name. The idea would be to use this new class for customization, and also to define WindowsPath and PosixPath sub-classing this new class. By default PathPlus would be an empty class. I do not know if this works well. Maybe my idea is a bad one.

    *Christophe BAL* *Enseignant de mathématiques en Lycée **et développeur Python amateur* *--- *French math teacher in a "Lycée" **and **Python **amateur developer\

    2015-05-06 13:05 GMT+02:00 Antoine Pitrou \report@bugs.python.org\:

    > > Antoine Pitrou added the comment: > > The Path classes were not designed to be subclassable by the user. > I'm not against making subclassing easier, but someone will have to > propose a viable approach for that. > > ---------- > versions: +Python 3.5 -Python 3.4 > > > Python tracker \report@bugs.python.org\ > \http://bugs.python.org/issue24132\ > >

    ----------


    Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue24132\


    pfmoore commented 9 years ago

    For that type of function, I'd suggest you use a standalone function rather than subclassing and methods or operator overloading. You don't gain enough to be worth the complexity of having to subclass path objects. And duck typing means that your function works for any subclass of (Pure)Path without change.

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago

    I don't agree with you. I prefer to add new functionalities to the paths I use. This is the power of OOP. It is easier and cleaner to use *mypath.common_with(otherpath) than \common_with(*mypath, **other path) .

    Python is highly OOP, so you can't say *"don't use subclassing in your case"*. As a user, I should have the possibility to use the method I want.

    Another example is the use of *onepath - anotherpath instead of \onepath.relative_to(*another path) . That's the power of the magic method to add this kind of feature.

    *Christophe BAL* *Enseignant de mathématiques en Lycée **et développeur Python amateur* *--- *French math teacher in a "Lycée" **and **Python **amateur developer\

    2015-05-06 20:21 GMT+02:00 Paul Moore \report@bugs.python.org\:

    Paul Moore added the comment:

    For that type of function, I'd suggest you use a standalone function rather than subclassing and methods or operator overloading. You don't gain enough to be worth the complexity of having to subclass path objects. And duck typing means that your function works for any subclass of (Pure)Path without change.

    ----------


    Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue24132\


    pfmoore commented 9 years ago

    I have no problem with that - it's a style choice certainly.

    As I said, I'd like to see simpler subclassing of pathlib objects. I just think it'll be quite hard to do (given the complexities of classes for Windows/Unix as well as pure and concrete paths). So if it's just about examples like this, I personally would take the easier route and just go with standalone functions. If someone else felt strongly enough to design and implement a subclassing solution, that's fine though.

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago

    Are you the author of path lib ?

    *Christophe BAL* *Enseignant de mathématiques en Lycée **et développeur Python amateur* *--- *French math teacher in a "Lycée" **and **Python **amateur developer\

    2015-05-06 21:01 GMT+02:00 Paul Moore \report@bugs.python.org\:

    Paul Moore added the comment:

    I have no problem with that - it's a style choice certainly.

    As I said, I'd like to see simpler subclassing of pathlib objects. I just think it'll be quite hard to do (given the complexities of classes for Windows/Unix as well as pure and concrete paths). So if it's just about examples like this, I personally would take the easier route and just go with standalone functions. If someone else felt strongly enough to design and implement a subclassing solution, that's fine though.

    ----------


    Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue24132\


    pfmoore commented 9 years ago

    Are you the author of path lib ?

    Nope, that's Antoine.

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 9 years ago

    OK. I will try to find a way to achieve an easier and cleaner way to sub class pathlib.Path and co.

    What is the good way to propose a patch ?

    *Christophe BAL* *Enseignant de mathématiques en Lycée **et développeur Python amateur* *--- *French math teacher in a "Lycée" **and **Python **amateur developer\

    2015-05-06 21:09 GMT+02:00 Paul Moore \report@bugs.python.org\:

    Paul Moore added the comment:

    > Are you the author of path lib ?

    Nope, that's Antoine.

    ----------


    Python tracker \report@bugs.python.org\ \http://bugs.python.org/issue24132\


    pfmoore commented 9 years ago

    What is the good way to propose a patch ?

    If you have a patch, attach it here, and it will get reviewed.

    5f0d55c6-d05f-44fb-8b8f-1a397f26ff4f commented 9 years ago

    If I were designing pathlib from scratch, I would not have a separate Path class. I would instead do something like this:

    In pathlib.py:

        if os.name == 'nt':
            Path = WindowsPath
        else:
            Path = PosixPath

    Alternatively, Path() could be a factory function that picks one of those classes at runtime.

    Of course, that still leaves the issue of where to put the method implementations which currently live in Path. We could change the name of Path to _Path and use the code above to continue providing a Path alias, but that might be too confusing. Another possibility is to pull those methods out into top-level functions and then alias them into methods in WindowsPath and PosixPath (perhaps using a decorator-like-thing to pass the flavor, instead of attaching it to the class).

    The main thing, though, is that Path should not depend on its subclasses. That really strikes me as poor design, since it produces issues like this one.

    287a392c-1882-4f2f-bebc-78a5b678f680 commented 6 years ago

    Using a set of paths with special properties and formats in a project, thought "the cleanest oop way to do this is try out python's oop paths in pathlib". Subclassed Path to implement my extra (non platfor specific) properties and fell at the first hurdle because of this issue...

    for me pathlib does not provide oop paths if i can't subclass Path, for whatever reason.

    reverted to treating paths as strings and writing functions to handle my special path properties and formats.

    i was also surprised when i found another bug report on this issue that said it was closed for 3.7, great i thought this has been solved, but no, the other report was closed because it was about the same issue as this ancient report.

    pfmoore commented 6 years ago

    @elguavas the problem is, no-one has proposed a patch. There's not likely to be much movement on this until someone provides one.

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 6 years ago

    For the moment, you can take a look at this little script that acheives subclassing of Path : https://github.com/bc-python/mistool/blob/master/mistool/os_use.py (search for class Path).

    Le 08/11/2017 à 09:55, Paul Moore a écrit :

    Paul Moore \p.f.moore@gmail.com\ added the comment:

    @elguavas the problem is, no-one has proposed a patch. There's not likely to be much movement on this until someone provides one.

    ----------


    Python tracker \report@bugs.python.org\ \https://bugs.python.org/issue24132\


    114124f8-a54e-4a8a-a5ba-52165f132225 commented 6 years ago

    Mistyping : /search for class PPath/ with two P.

    Le 08/11/2017 à 13:59, Christophe BAL a écrit :

    Christophe BAL \projetmbc@gmail.com\ added the comment:

    For the moment, you can take a look at this little script that acheives subclassing of Path : https://github.com/bc-python/mistool/blob/master/mistool/os_use.py (search for class Path).

    Le 08/11/2017 à 09:55, Paul Moore a écrit : > Paul Moore \p.f.moore@gmail.com\ added the comment: > > @elguavas the problem is, no-one has proposed a patch. There's not likely to be much movement on this until someone provides one. > > ---------- > > > Python tracker \report@bugs.python.org\ > \https://bugs.python.org/issue24132\ > ----------


    Python tracker \report@bugs.python.org\ \https://bugs.python.org/issue24132\


    287a392c-1882-4f2f-bebc-78a5b678f680 commented 6 years ago

    @paul.moore is the original contributor mia? i seem to remember pathlib as once being marked 'provisional', i think it should have stayed that way until this problem was resolved. easy to say i know ;) when i don't have a patch.

    @projetmbc yes i found various work-arounds on the web and decided to not use any of them. really i feel this should be fixed as it's a jarring inconsistency with naturally expected behaviour for a class in python.

    so i added my report to this as a topic bump because i don't think this should be forgotten about and in case anyone might come up with an idea how to fix it.

    1cc07159-81c3-42e2-8d7a-b6940f9aab76 commented 6 years ago

    Look at the architecture of Rio in Ruby (also ported to Squeak/Smalltalk)

    Leave Path to handle path stuff, and have another class to handle Platform stuff.

    https://rubygems.org/gems/rio/versions/0.6.0

    1e658ab0-5ece-486a-a504-b31bb432e87b commented 6 years ago

    Hi all,

    I made a pull request proposing a fix for this issue. There is still quite a lot to be done:

    I will try to fix those by the end of the week.

    The patch mainly consists of two things:

    Ideally I would like _PurePath to become a public class, but I could not come up with a proper name. Any feedback is more than welcome =]

    114124f8-a54e-4a8a-a5ba-52165f132225 commented 6 years ago

    Hello.

    What about AbstractPath instead of _PurePath ?

    Le 28/03/2018 à 02:30, qb-cea a écrit :

    qb-cea \quentin.bouget@cea.fr\ added the comment:

    Hi all,

    I made a pull request proposing a fix for this issue. There is still quite a lot to be done:

    • I exposed some variables (and probably methods too) that used to be hidden;
    • I did not update the documentation;
    • I did not add a proper test.

    I will try to fix those by the end of the week.

    The patch mainly consists of two things:

    • having Path (resp. PurePath) be a variable pointing at either (Pure)PosixPath or (Pure)WindowsPath, depending on the platform (like Kevin Norris suggested);
    • introducing two new abstract classes _PurePath and ConcretePath from which PurePosixPath, PureWindowsPath and PosixPath, WindowsPath classes inherit;
    • removing the _Flavor classes, and redistributing their method to platform-specific classes.

    Ideally I would like _PurePath to become a public class, but I could not come up with a proper name. Any feedback is more than welcome =]

    ----------


    Python tracker \report@bugs.python.org\ \https://bugs.python.org/issue24132\


    1e658ab0-5ece-486a-a504-b31bb432e87b commented 6 years ago

    What about AbstractPath instead of _PurePath ?

    I will use this, thanks.

    4b0c5ead-8181-475a-af87-9342ce46c4e3 commented 4 years ago

    I'm taking another look at making pathlib extensible. There's some discussion here: https://discuss.python.org/t/make-pathlib-extensible/3428

    List or preparatory bugfixes and tidy-ups: https://docs.google.com/spreadsheets/d/1TicFDMudKKA6CZcrscg1Xq9kt5Q8To8y0hADGw9u11I/edit#gid=0

    1e658ab0-5ece-486a-a504-b31bb432e87b commented 3 years ago

    Hi,

    Thanks for reviving this! Feel free to reuse any code I wrote in my PR (or the whole PR itself), I do not think I will ever get around to finishing this work myself.

    4b0c5ead-8181-475a-af87-9342ce46c4e3 commented 3 years ago

    Progress report:

    I've been working on tidying up the pathlib internals over the 3.9 and 3.10 releases. We're now in a position where:

    The internal abstractions are now much tighter, which allows us to begin refactoring them with confidence!

    The next step is to remove accessors, in bpo-43012.

    After that I'll finally be in a position to start working on this bug!

    e78ae322-893d-4a14-826e-e6d8794d8394 commented 3 years ago

    I agree this would be nice. For now, I'm doing this as a hack:

    class Path(type(pathlib.Path())):
        ...
    miss-islington commented 2 years ago

    New changeset 08f8301b21648d58d053e1a513db8ed32fbf37dd by Barney Gale in branch 'main': bpo-43012: remove pathlib._Accessor (GH-25701) https://github.com/python/cpython/commit/08f8301b21648d58d053e1a513db8ed32fbf37dd

    4b0c5ead-8181-475a-af87-9342ce46c4e3 commented 2 years ago

    If/when python/issues-test-cpython#31691 lands, I think this bug can be resolved: the original repro case will no longer raise AttributeError, and subclasses will be able to customize behaviour without needing to define further "flavour" or "accessor" subclasses.

    ghost commented 2 years ago

    I agree this would be nice. For now, I'm doing this as a hack:

    class Path(type(pathlib.Path())):
        ...

    Worth noting that this confuses mypy even when adding # type: ignore, so now I'm doing this instead:

    import pathlib
    from typing import TYPE_CHECKING
    
    # We can't subclass pathlib.Path directly (https://github.com/python/cpython/issues/68320)
    if TYPE_CHECKING:
        class BasePath(pathlib.Path):
            pass
    else:
        class BasePath(type(pathlib.Path())):
            pass
    
    class Path(BasePath):
       # Actual custom path class goes here
    mzipay commented 1 year ago

    Sorry if I'm "late to the party," but ALL of the discussion from non-mannequins seems (to me) to miss the point entirely.

    The pathlib module fails (grossly, IMO) to respect two key "Pythonic" principles:

    . If the implementation is hard to explain, it's a bad idea.