migrate node fs paths to pathlib primary and reorg ctors/from_parent by RonnyPfannschmidt · Pull Request #8037 · pytest-dev/pytest

RonnyPfannschmidt · 2020-11-15T15:35:44Z

@pytest-dev/core with this i'm starting my continuation of the removal of both, too smart ctors annd migrating fspath to a pathlib version

its going to be a draft for a while while i iterate and figure the exact details, please scrutinize meanwhile ^^

RonnyPfannschmidt · 2020-11-15T19:19:03Z

while fixing the typing issues it seems i introduced a bug in collection i keep missing atm

im under the impression that resolving it will require a new type of node construction for sessions in order to avoid certain issues

RonnyPfannschmidt · 2020-11-15T21:44:46Z

the failure in the plugins testing is due to a unavoidable issue with pytest-flakes, i'll check out whether it an be coordinated away

asottile

hmm yeah the break is unfortunate, I wonder if there's some trick we could do with taking both fspath / fs_path parameters for some time and then deprecate / adjust? might make for a smoother transition for plugins (though of course, way more work for us!)

most of my comments are little type things, overall seems good 👍

src/_pytest/doctest.py

src/_pytest/main.py

src/_pytest/nodes.py

nicoddemus

Overall is looking good! Left a few comments. 👍

nicoddemus · 2020-11-21T13:05:15Z

src/_pytest/nodes.py


        #: Filesystem path where this node was collected from (can be None).
-        self.fspath = fspath or getattr(parent, "fspath", None)
+        self.fs_path = fs_path


Being a bit nitpicky, but fs_path and fspath are too close to one another, I fear users will get the two confused all the time.

How about: path for the Path version? After all the fs prefix is a bit redudant.

I agree with @nicoddemus, the name fs_path is not very pretty.

One thing against just path is that someday we'll also want to migrate our hooks to Path and their the py.path param is always path so the Path param would need a different name. And path is not very distinguishable either. But I can't think of something else.

How about file_path? This is always a file right?

there are situations where it ought to be a folder

external collection utilities

collection roots (a concept i intend to introduce as extension of the testpaths configuration (i think it needs a ticket)

Packages ought to use it as well

src/_pytest/nodes.py

nicoddemus · 2020-11-21T13:20:43Z

src/_pytest/nodes.py

-        self.fspath = fspath
-
-        session = session or parent.session
+                name = str(rel)


This will end up with a name containing \ on Windows because we are no longer doing name = name.replace(os.sep, SEP). Is that intentional?

Also str seems redudant:

>>> local("src/pytest").relto(local(".")) 'src\\pytest'

relto is from py.path.local - relative pathlib paths are relative

Ahh I see, thanks, but does the comment about name.replace still apply?

>>> local("src/pytest").relto(local(".")) 'src\\pytest'

Ahh OK so this needs fixing then. 👍

nicoddemus · 2020-11-21T13:22:44Z

src/_pytest/nodes.py

-            nodeid = self.fspath.relto(session.config.rootdir)
+            assert parent is not None
+            session = parent.session
+            relp = session._bestrelpathcache[known_path]


nicoddemus · 2020-11-21T13:23:37Z

src/_pytest/nodes.py

+            assert parent is not None
+            session = parent.session
+            relp = session._bestrelpathcache[known_path]
+            if not relp.startswith(".." + os.sep):


Can we have a comment here explaining the purpose of this?

Gentle ping

src/_pytest/python.py

bluetech

Not a full review, just some comment.

bluetech · 2020-11-21T17:54:12Z

src/_pytest/main.py

    def __init__(self, config: Config) -> None:
        super().__init__(
-            config.rootdir, parent=None, config=config, session=self, nodeid=""
+            fs_path=Path(config.rootdir),


Suggested change

fs_path=Path(config.rootdir),

fs_path=config.rootpath,

src/_pytest/doctest.py

bluetech · 2020-11-21T17:55:24Z

src/_pytest/main.py

-                        key = (type(x), x.fspath)
-                        if key in node_cache2:
-                            yield node_cache2[key]
+                        keya = (type(x), x.fspath)


I suggest key2 to match node_cache2.

bluetech · 2020-11-21T17:56:39Z

src/_pytest/main.py

-                        key = (type(node), node.nodeid)
-                        if key in matchnodes_cache:
-                            rep = matchnodes_cache[key]
+                        keyb = (type(node), str(node.nodeid))


Here I would call it key_matchnodes.

Why is the str(node.nodeid) needed? Isn't nodeid already an str?

src/_pytest/nodes.py

bluetech · 2020-11-21T18:09:46Z

src/_pytest/nodes.py


        #: Filesystem path where this node was collected from (can be None).
-        self.fspath = fspath or getattr(parent, "fspath", None)
+        self.fs_path = fs_path


I agree with @nicoddemus, the name fs_path is not very pretty.

One thing against just path is that someday we'll also want to migrate our hooks to Path and their the py.path param is always path so the Path param would need a different name. And path is not very distinguishable either. But I can't think of something else.

src/_pytest/nodes.py

also helps to implement pytest-dev/pytest#8037

RonnyPfannschmidt · 2020-11-24T05:36:45Z

as soon as the pytest-flakes maintainer fins some time to merge/release the pipeline should get green, this still is a breaking change in terms of requiring people to actually write proper ctors

im thinking of reintroducing implying for a little while and using the pytest-flakes pr as a example for propper ctors

cc @nicoddemus @bluetech @asottile

nicoddemus · 2020-11-26T13:36:52Z

Cool thanks for the follow up

im thinking of reintroducing implying for a little while...

Not sure I follow can you please clarify that?

RonnyPfannschmidt · 2020-11-26T13:47:52Z

Aka having the node ctors imply things and warning about it so plugins can migrate

nicoddemus · 2020-11-26T16:46:35Z

Ahh got it, thanks!

RonnyPfannschmidt · 2020-12-20T20:28:20Z

@nicoddemus @bluetech it seems like the fragile base class issue is hitting this pr and many plugins way harder than what id hope for

im not sure whether we should fix plugisn or reiterate the approach

nicoddemus · 2020-12-23T11:02:05Z

fragile base class

What you mean exactly, in terms of the number of arguments that can be passed?

im thinking of reintroducing implying for a little while and using the pytest-flakes pr as a example for propper ctors

This might be the best way to go, as at least plugins will continue to work, and we can issue a warning for a few releases suggesting the change. With 7.0 we can then make the final breaking change.

RonnyPfannschmidt · 2020-12-31T12:37:15Z

@nicoddemus hitting another case of rebase demonstrating why primarily acceptance tests is a massive problem, will debug sometime later

Co-authored-by: Ran Benita <ran@unusedvar.com>

Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>

for more information, see https://pre-commit.ci

bluetech

I think it would be easier to evaluate this PR if it were split into at least two parts:

The implied-arguments deprecation
The pathlib conversions

Because they seem not really related, and it will be easier to review each one of these separately. But if it's too much work that's OK.

Regarding the implied-arguments deprecation - the code looks good to me but at a higher level I'm still not quite sure what is the reason for the deprecation, or of from_parent in general?

Regarding the pathlib conversion, that's great, I think it's the last major piece 👍 But the code is a little hard to review, having a PR which just does this part would really help me at least. Also regarding the name, I prefer path over fs_path, it's quite minor but I don't like how fs_path looks (in the hookspecs we stuck with path and fspath).

bluetech · 2021-01-16T18:42:38Z

src/_pytest/nodes.py

        "__dict__",
    )

+    name: str


Is mypy not able to figure these out on its own?

bluetech · 2021-01-16T18:44:41Z

src/_pytest/nodes.py

+        fspath: Optional[py.path.local] = kw.pop("fspath", None)
+
+        if fspath is not None:
+            assert fs_path is None


Suggested change

assert fs_path is None

assert fs_path is None, "Specify either fspath or fs_path, not both"

bluetech · 2021-01-16T18:58:54Z

changelog/8037.deprecation.rst

@@ -0,0 +1 @@
+Deprecate implying node ctor arguments.


I think if I read this sentence as a user I would not be able to grok what it's about, so some more details would be good here, like what exactly is deprecated, and how to fix.

Also we usually add a corresponding note in the docs/deprecations.rst file (can just be a copy/paste of the changelog entry mostly).

Agreed. 👍

bluetech · 2021-01-16T19:00:10Z

src/_pytest/deprecated.py

+    "implying {type.__name__}.{arg} from parent.{arg} has been deprecated\n"
+    "please use the Node.from_parent to have them be implied by the superclass named ctors",


Suggested change

"implying {type.__name__}.{arg} from parent.{arg} has been deprecated\n"

"please use the Node.from_parent to have them be implied by the superclass named ctors",

"Implying {type.__name__}.{arg} from parent.{arg} has been deprecated.\n"

"Please use Node.from_parent to have them be implied by the superclass named constructors.",

src/_pytest/doctest.py

RonnyPfannschmidt · 2021-01-17T08:29:03Z

src/_pytest/nodes.py

-        config: Optional[Config] = None,
-        session: Optional["Session"] = None,
-        nodeid: Optional[str] = None,
+        parent: Node,


this one i missed while restoring ctor based implication

nicoddemus · 2021-01-17T11:52:46Z

changelog/8037.feature.rst

@@ -0,0 +1 @@
+Introduce ``Node.fs_path`` as pathlib replacement for ``Node.fspath``


Suggested change

Introduce ``Node.fs_path`` as pathlib replacement for ``Node.fspath``

Introduce ``Node.fs_path`` as pathlib replacement for ``Node.fspath``.

Also I would add another paragraph:

This is part of our continuous effort of eventually phasing out pytest's dependency to `py <https://py.readthedocs.io/en/latest/>`__.

nicoddemus · 2021-01-17T11:55:12Z

src/_pytest/nodes.py

+        parent: Optional["Node"],
        config: Optional[Config] = None,
-        session: "Optional[Session]" = None,
-        fspath: Optional[py.path.local] = None,


By dropping fspath (the py.path.local variant) aren't we breaking the API? If so I think a better option would be to still support fspath, converting it internally to fs_path and issue a deprecation warning.

nicoddemus · 2021-01-17T11:56:04Z

src/_pytest/nodes.py

+    def fspath(self) -> py.path.local:
+        """Filesystem path where this node was collected from (can be None).
+
+        Since pytest 6.2, prefer to use `fs_path` instead which returns a `pathlib.Path`


Suggested change

Since pytest 6.2, prefer to use `fs_path` instead which returns a `pathlib.Path`

Since pytest 6.2, prefer to use `fs_path` instead which returns a :class:`pathlib.Path`

RonnyPfannschmidt · 2021-01-17T13:17:43Z

the overall input here has convinced me to do the extra work of extraction and splitting

originally i planned to simplify things by dropping implication at the same time, but all i accomplished is learning that i had to bring it back, so im going to reiterate today and take what i learned to do a number of prs to make things prettier

as PyObjMixin is always supposed to be mixed in the mro before nodes.Node the behavior doesn't change, but all the typing information carry over to help mypy. extracted from pytest-dev#8037

RonnyPfannschmidt · 2021-03-27T14:50:56Z

closing as resolved in other prs by me, thanks for the help

RonnyPfannschmidt added topic: collection related to the collection phase type: backward compatibility might present some backward compatibility issues which should be carefully noted in the changelog labels Nov 15, 2020

RonnyPfannschmidt requested review from asottile, bluetech and nicoddemus and removed request for nicoddemus November 15, 2020 20:25

RonnyPfannschmidt added the type: removal marks the actual removal of a feature, usually done in major releases label Nov 15, 2020

asottile reviewed Nov 19, 2020

View reviewed changes

src/_pytest/doctest.py Outdated Show resolved Hide resolved

src/_pytest/main.py Outdated Show resolved Hide resolved

src/_pytest/nodes.py Outdated Show resolved Hide resolved

RonnyPfannschmidt force-pushed the fspath-as-pathlib branch from 7de346a to c75ee78 Compare November 20, 2020 22:27

nicoddemus approved these changes Nov 21, 2020

View reviewed changes

bluetech reviewed Nov 21, 2020

View reviewed changes

RonnyPfannschmidt changed the title ~~migrate node fs paths to pathlib primary and reor ctors/from_parent~~ migrate node fs paths to pathlib primary and reorg ctors/from_parent Nov 22, 2020

RonnyPfannschmidt added a commit to RonnyPfannschmidt/pytest-flakes that referenced this pull request Nov 23, 2020

switch to cooperative ctors and correct node trees to future proof

1dfc587

also helps to implement pytest-dev/pytest#8037

RonnyPfannschmidt mentioned this pull request Nov 23, 2020

switch to cooperative ctors and correct node trees to future proof asmeurer/pytest-flakes#40

Merged

bluetech mentioned this pull request Dec 14, 2020

hookspec: add pathlib.Path alternatives to py.path.local parameters in hooks #8144

Merged

RonnyPfannschmidt force-pushed the fspath-as-pathlib branch from 47833e9 to 57b1f33 Compare December 21, 2020 17:29

RonnyPfannschmidt force-pushed the fspath-as-pathlib branch from 2fe61c1 to 0ee0897 Compare December 31, 2020 12:18

RonnyPfannschmidt force-pushed the fspath-as-pathlib branch 3 times, most recently from 576997a to 6934346 Compare January 4, 2021 23:59

RonnyPfannschmidt force-pushed the fspath-as-pathlib branch from 6934346 to de835f2 Compare January 15, 2021 21:29

migrate node fs paths to pathlib primary and prepare to drop smart ctors

424f826

Co-authored-by: Ran Benita <ran@unusedvar.com>

RonnyPfannschmidt force-pushed the fspath-as-pathlib branch from de835f2 to 424f826 Compare January 15, 2021 21:31

RonnyPfannschmidt marked this pull request as ready for review January 15, 2021 21:35

RonnyPfannschmidt requested review from asottile, bluetech and nicoddemus January 15, 2021 21:36

RonnyPfannschmidt and others added 3 commits January 16, 2021 10:40

fixup

ebc5d75

drop unneeded annotation

b7b4162

Apply suggestions from code review

9749629

Co-authored-by: Bruno Oliveira <nicoddemus@gmail.com>

RonnyPfannschmidt mentioned this pull request Jan 16, 2021

tighten Node.parent to Collector and resolve the resulting typing issues #8244

Open

[pre-commit.ci] auto fixes from pre-commit.com hooks

e9936de

for more information, see https://pre-commit.ci

bluetech reviewed Jan 16, 2021

View reviewed changes

RonnyPfannschmidt commented Jan 17, 2021

View reviewed changes

nicoddemus reviewed Jan 17, 2021

View reviewed changes

RonnyPfannschmidt mentioned this pull request Jan 17, 2021

alter the PyObjMixin to carry over typing information from Node #8248

Merged

Base automatically changed from master to main March 9, 2021 20:40

RonnyPfannschmidt closed this Mar 27, 2021

h-vetinari mentioned this pull request Jun 29, 2021

add more docs on the fspath deprecation #8445

Closed

	assert fs_path is None
	assert fs_path is None, "Specify either fspath or fs_path, not both"

		"implying {type.__name__}.{arg} from parent.{arg} has been deprecated\n"
		"please use the Node.from_parent to have them be implied by the superclass named ctors",

		@@ -0,0 +1 @@
		Introduce ``Node.fs_path`` as pathlib replacement for ``Node.fspath``

	Since pytest 6.2, prefer to use `fs_path` instead which returns a `pathlib.Path`
	Since pytest 6.2, prefer to use `fs_path` instead which returns a :class:`pathlib.Path`

Uh oh!

Conversation

RonnyPfannschmidt commented Nov 15, 2020

Uh oh!

RonnyPfannschmidt commented Nov 15, 2020

Uh oh!

RonnyPfannschmidt commented Nov 15, 2020

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RonnyPfannschmidt commented Nov 24, 2020

Uh oh!

nicoddemus commented Nov 26, 2020

Uh oh!

RonnyPfannschmidt commented Nov 26, 2020

Uh oh!

nicoddemus commented Nov 26, 2020

Uh oh!

RonnyPfannschmidt commented Dec 20, 2020

Uh oh!

nicoddemus commented Dec 23, 2020

Uh oh!

RonnyPfannschmidt commented Dec 31, 2020

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!