Pytest: doc: pytest_ignore_collect: wrong signature

[blueyed]

It says to provide a string, but (at least in some place) is a py.path.local.

Ref: https://github.com/pytest-dev/pytest/blob/e3e57a755b03f8cd9219c4db490c180fa0543785/src/_pytest/hookspec.py#L191

[blueyed]

Also affected probably: pytest_collect_directory

[DamianSkrzypczak]

Can I give it a try? It looks like nice ticket for beginning of my oss contributing adventure :smiley:

[blueyed]

@DamianSkrzypczak
Yes, go ahead.. :)

[DamianSkrzypczak]

@blueyed
I see two different approaches in which this can be done:

1. OR approach like in here:
   https://github.com/pytest-dev/pytest/blob/e3e57a755b03f8cd9219c4db490c180fa0543785/src/_pytest/hookspec.py#L576

2. (but because above style occurs only once in whole codebase) alternatively - no type given with proper explanation in param description:

:param path: the path to analyze (can be ``str`` or ``py.path.local``)

Example of both approaches as built sphinx doc:

If it wouldn't be single occurrence, I would choose first one, but because this can determine future style in such situations (this single occurrence is special case with deprecation and allows None which can be there for its falsy properties, but here there are two distinct object types), I would prefer for decision to be done for me by someone more experienced with project :)

[nicoddemus]

I actually think we should update the code so it is always a str. As it stands, users are forced to always either cast to str or py.path (because the argument can be either), so this is error-prone.

[DamianSkrzypczak]

@blueyed
@nicoddemus

Then I should I just make conversion from py.path.local to string in(or before) each call of pytest_ignore_collect and pytest_collect_directory to ensure that it's getting string value?
(of course whenever it's getting string, conversion will not be necessary)

[nicoddemus]

Then I should I just make conversion from py.path.local to string in(or before) each call of pytest_ignore_collect and pytest_collect_directory to ensure that it's getting string value?

Exactly. :+1:

(of course whenever it's getting string, conversion will not be necessary)

I would go ahead and just call str() on every call, regardless if it is a string or not; str() on a str object is a noop anyway. 😁

[DamianSkrzypczak]

@nicoddemus
Well, I agree but isn't str(str) make it a little obscured?
str(x) with x being already a string value isn't so obvious when you're reading code, IMO it's last thing you're thinking of when you're reading it ^.^ it's secure but pretty implicit.

Are you still sure that it's best choice to str() everything despite it's original type?

[nicoddemus]

str(x) with x being already a string value isn't so obvious when you're reading code, IMO it's last thing you're thinking of when you're reading it ^.^ it's secure but pretty implicit.

It is just that probably at the point of the call, it is not entirely obvious that the value is actually a string; it probably is being taken from the property of another object, for example item.fspath, in which case makes sense to use str(item.fspath) to ensure we are calling the hook with a str argument.

Are you still sure that it's best choice to str() everything despite it's original type?

My point is that it is the simplest approach; I would rather see:

config.ihook.pytest_collect_directory(path=str(item.fspath), parent=parent)

Than:

config.ihook.pytest_collect_directory(path=str(item.fspath) if not isinstance(item.fspath, str) else item.fspath, parent=parent)

That's all. 😁

[blueyed]

This will not be backward compatible then.
And some people might actually rely on the object instead of the string.

Apart from that it seems good to pass an object if we have it already.
But I also think it is better to have strings in generl for consistency/simplicity.

[nicoddemus]

And some people might actually rely on the object instead of the string.

I doubt it, if we are sometimes passing str and other times py.local objects, hooks that expect py.local will break when str objects are being passed. I suspect every hook implementation is using path = str(path) anyway because of this inconsistency right now, that's why I think changing to always pass str objects is the correct approach.

Also keep in mind that we don't want to expose py.local further, as we have plans to replace it entirely by pathlib.Path objects in the (distant) future.

[DamianSkrzypczak]

@nicoddemus OK! Sounds fine for me. PR on the way.

It's worth mentioning that I found two additional hooks which expect path parameter but as py.path.local:

• pytest_collect_file
• pytest_pycollect_makemodule

Right now, I will assume that it doesn't matter and I will change only pytest_collect_directory and pytest_ignore_collect calls because I don't really want this discussion to last forever :D and it would be great to have PR, even if this PR will change over time.

[blueyed]

@DamianSkrzypczak
Starting with a PR already is fine, but it should aim to fix it everywhere then.

@nicoddemus
I agree that strings are better, but just wanted to mention that this will break existing plugins that expect/use a py.path object there.

[nicoddemus]

I agree that strings are better, but just wanted to mention that this will break existing plugins that expect/use a py.path object there.

I understand and appreciate the concern, but I believe those plugins cannot be working today without converting path to what they want to work with (either str or py.local), given that we sometimes pass path as str and others as py.local (if I understand the original issue correctly, that is).

So in the same pytest version, someone who implements pytest_ignore_collect must either convert the input to str or py.local (according to their preference), because pytest is not consistent and will sometimes call it with a str parameter while call it with py.local other times.

[blueyed]

My impression was that pytest would be at least consistent _per_ hook, i.e. somebody might use str.strpath (or some other attributes of py.path), which would then break.

[DamianSkrzypczak]

@blueyed
@nicoddemus

I will open PR soon but I will also wait for final decision :smile:

I don't really think I have anything more to add, maybe only my opinion
but I don't feel project-experienced enough to really think its valuable...
at least not until someone will ask for it :smile:

[nicoddemus]

My impression was that pytest would be at least consistent per hook

Then I'm confused... the original description in this issue reads:

It says to provide a string, but (at least in some place) is a py.path.local.

This leads me to believe that the same hook would sometimes be called with str objects, and others with py.path.local. Isn't that the case?

[nicoddemus]

OK, took the time to review this. In the case of pytest_ignore_collect, every call that I can see in pytest passes a py.local.path instance. So for pytest_ignore_collect, it is clear we should only update the docstring, as every call is getting a py.local.path.

Checked pytest_collect_directory, pytest_collect_file, and pytest_pycollect_makemodule: same thing, we always pass a py.local.path instance.

So in the end, sorry about the confusion, I misunderstood the issue.

@DamianSkrzypczak: in summary, we just need to update the docstring of those hooks to state that they receive py.local.path objects, and not str objects as stated in the docs.

[blueyed]

This leads me to believe that the _same_ hook would sometimes be called with str objects, and others with py.path.local. Isn't that the case?

I haven't checked the details, just reported it - otherwise it would have been a PR already.. ;)
Sorry for not being clear, but that was what I've implied with "at least in some place".

[DamianSkrzypczak]

@blueyed
@nicoddemus

PR open, I did some additional job on code that I though should also be documented properly (I added some doc and fixed one more occurrence), everything is described in PR so please take a look :smile_cat: