pytest 7: path vs. fspath, pathlib vs. py.path

[The-Compiler]

I've just been looking through the changelog for pytest 7 and noticed something I'd like to discuss before the release:

In #8144, @bluetech added pathlib alternatives to various hooks, and called them fspath, with the old py.path.local alternatives keeping their path name.

However, in #8251, @RonnyPfannschmidt added a pathlib attribute to Node, and called it path, with the old py.path one being called fspath...

We then reverted the Node.fspath deprecation in #8903 after some discussion in #8821, but kept the new path attribute...

However, the naming inconsistency seems like something that could confuse people. What to do? Keep it as is, or revert the new attribute entirely until we figured out the rest of the deprecation? Make it private perhaps?

[bluetech]

Definitely confusing. The py.path situation was: nodes use fspath, hooks use path. Since pathlib needed to use new names, the choice was either an entirely new name, or the reverse inconsistency. We did the latter.

[nicoddemus]

Hmm what if we adopt a new name for pathlib paths that differs from what we used so far, like filepath? I don't think we have used that yet, so any new pathlib attribute could be named filepath or dirpath accordingly.

[The-Compiler]

I think I'd prefer that personally, just because the transient situation is already confusing enough as-is...

Then again, maybe it's a bit late to introduce such a big change now. I haven't followed the original discussion, but if the reverse inconsistency was what was decided upon there, then I don't want to rehash this.

[nicoddemus]

Then again, maybe it's a bit late to introduce such a big change now.

I don't think so, those changes have not been released to the wild yet.

@RonnyPfannschmidt @bluetech what do you think?

[bluetech]

The confusion occurs in 3 hooks:

• pytest_ignore_collect - can be either file or directory - filepath/dirpath won't work.
• pytest_collect_file - filepath could work.
• pytest_pycollect_makemodule - module path - maybe modpath?

[The-Compiler]

So what should we do here for 7.0? If we go for the current inconsistency, that should at least be properly documented somewhere... Or perhaps, given that we already reverted the Node.fspath deprecation in #8903 due to some unsolved questions, we should go one step further, and make Node.path private (or at least undocumented) so we can release 7.0 yet have some more time to figure out how to proceed there, perhaps once we have a clearer picture in mind?

Of course the fspath name for hooks would be more or less set in stone then.

[bluetech]

IMO, current state is a bit confusing, but other names won't be much better, so I'd just leave it.

[RonnyPfannschmidt]

I agree its a bitter pill, but the other pills seem just as bitter

[The-Compiler]

Ok. I think we could do better in terms of documentation. Right now on latest, it seems like neither path nor fspath is documented, for example... I'll try to open a PR tomorrow or Friday, got distracted by the doc build failure I found today instead... (#9336)

[nicoddemus]

Agreed, thanks @The-Compiler

[nicoddemus]

Was thinking about this a bit more, let me throw another idea into the mix to see what you folks think:

IMO, current state is a bit confusing, but other names won't be much better, so I'd just leave it.

Recently we have opted for longer/more descriptive names in general, and using _ more to separate words. We can reflect that for the new attributes then:

• Files: file_path
• Directories: dir_path
• When both are possible: file_sys_path

They are longer, but easy to read, and also easier to explain when we have a legacy path (fspath, path) vs a new-style path: file_sys_path, file_path. Unless you folks think the longer names are too clunky, that might be easier to explain.

[The-Compiler]

Not entirely sure. This will delay things a bit further I assume, but it would be a cleaner way forward, and also fit in nicely with e.g. tmpdir -> tmp_path as you say.

However, so far I'm still not convinced if we can come up with sensible new names really. Not sure about file_sys_path mostly, I don't understand sys in that context. file_sys as in filesystem? Seems to confusing due to the sys module and sys.path IMHO.

Maybe we can keep path for the Node argument and attribute though, and only find something new for hooks, like proposed by @bluetech earlier?

By all means, if the consensus is "let's stop bikeshedding this and go with #9341", that's fine by me too. Good naming is important, but we're in an unfortunate temporary situation with seemingly no good one answer...

[nicoddemus]

By all means, if the consensus is "let's stop bikeshedding this and go with #9341", that's fine by me too.

Yeah fine by me too. I worry because those naming decisions usually hang around for years, and plugin authors that need to support multiple pytest versions then have to deal with the differences too.

Btw, I believe it is fine to release 7.0.0rc1 regardless of the decision here TBH. Those are new attributes which will affect a very small percentage of installations, even if we change the new attributes/parameters in the final 7.0.0, it won't invalidate the testing of the release candidate.

[The-Compiler]

I'd be vary to release the rc before the change, if we decide to do it. Otherwise, plugins might start adjusting their code for the RC already (after all, we would like especially plugins or complex testsuites to test the RC!). If we then change things in an incompatible way just after the rc, I fear that will introduce more chaos.

[The-Compiler]

(whoops, misclicked, sorry for the noise!)

[nicoddemus]

Ahh you are absolutely right, I was thinking of an alpha/beta release, sorry!

[The-Compiler]

After thinking about this for some more, how do people feel about:

• pytest_ignore_collect: fspath -> collection_path (tricky one because it can be either a file or directory)
• pytest_collect_file: fspath -> file_path
• pytest_pycollect_makemodule: fspath -> mod_path
• pytest_report_header: startpath -> start_path (for better consistency with tmpdir -> tmp_path)
• pytest_report_collectionfinish: ditto
• Node.path: Keep path - we still have fspath as the old attribute (while path is the old argument for hooks), but it's some more code crunch, and I can't think of a better alternative

Agreed that - even if it delays things - we should aim to get this as right as we can in the situation. Like @nicoddemus said, it's going to stick around for a while and we won't be able to change it again for a while...

[nicoddemus]

👍 to everything!. in general, agree with the points made by @bluetech below. 😁

Good idea, just changing the hooks should be straightforward as those have not been published yet. If everybody agrees, I would be happy to open a PR.

[bluetech]

startpath should stay because it's under this name in Config (already released). The others look OK to me. Maybe mod_path should be module_path, because the hook name is makemodule.

[The-Compiler]

Great! Looks like we're coming to a conclusion then! 🎉

PR would sure be appreciated - I might get to it tomorrow, but if you have some time to work on this today, please go ahead!

[The-Compiler]

startpath should stay because it's under this name in Config (already released).

Wait, is it? Looking at the 6.2.x reference docs I don't see any startpath, and in the latest reference docs there's only Session.startpath which is new in 7.0.0, so we could still change it.

Do you mean rootpath by any chance?

[bluetech]

Do you mean rootpath by any chance?

Sorry, meant TerminalReporter.startpath. It is the value that is actually passed to these hooks. It's not documented.

[The-Compiler]

Given that it's undocumented and not public API, we should be free to either change it; or keep it as-is for compatibility without any confusion. So I'd still propose startpath -> start_path for consistency with tmpdir -> tmp_path and friends.