Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: pytest_ignore_collect: wrong signature #5171

Closed
blueyed opened this issue Apr 26, 2019 · 20 comments

Comments

Projects
None yet
3 participants
@blueyed
Copy link
Contributor

commented Apr 26, 2019

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

Ref:

:param str path: the path to analyze

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Also affected probably: pytest_collect_directory

@blueyed blueyed changed the title pytest_ignore_collect: wrong signature doc: pytest_ignore_collect: wrong signature Apr 27, 2019

@blueyed blueyed added the type: docs label Apr 27, 2019

@DamianSkrzypczak

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

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

@blueyed

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@DamianSkrzypczak
Yes, go ahead.. :)

@DamianSkrzypczak

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

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

  1. OR approach like in here:

    :param pytest.Item|None item:

  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:

image

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

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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. 👍

(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

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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 comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

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

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 1, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@blueyed
@nicoddemus

I will open PR soon but I will also wait for final decision 😄

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 😄

@nicoddemus

This comment has been minimized.

Copy link
Member

commented May 2, 2019

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

This comment has been minimized.

Copy link
Member

commented May 2, 2019

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 comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

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 added a commit to DamianSkrzypczak/pytest that referenced this issue May 2, 2019

doc: fix hooks 'path' parameter doc type
by changing it from str to py.path.local
(pytest-dev#5171)
@DamianSkrzypczak

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@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 😸

DamianSkrzypczak added a commit to DamianSkrzypczak/pytest that referenced this issue May 2, 2019

@nicoddemus nicoddemus closed this May 3, 2019

bors bot added a commit to rehandalal/therapist that referenced this issue May 10, 2019

Merge #69
69: Update pytest to 4.4.2 r=rehandalal a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **4.4.1** to **4.4.2**.



<details>
  <summary>Changelog</summary>
  
  
   ### 4.4.2
   ```
   =========================

Bug Fixes
---------

- `5089 &lt;https://github.com/pytest-dev/pytest/issues/5089&gt;`_: Fix crash caused by error in ``__repr__`` function with both ``showlocals`` and verbose output enabled.


- `5139 &lt;https://github.com/pytest-dev/pytest/issues/5139&gt;`_: Eliminate core dependency on &#39;terminal&#39; plugin.


- `5229 &lt;https://github.com/pytest-dev/pytest/issues/5229&gt;`_: Require ``pluggy&gt;=0.11.0`` which reverts a dependency to ``importlib-metadata`` added in ``0.10.0``.
  The ``importlib-metadata`` package cannot be imported when installed as an egg and causes issues when relying on ``setup.py`` to install test dependencies.



Improved Documentation
----------------------

- `5171 &lt;https://github.com/pytest-dev/pytest/issues/5171&gt;`_: Doc: ``pytest_ignore_collect``, ``pytest_collect_directory``, ``pytest_collect_file`` and ``pytest_pycollect_makemodule`` hooks&#39;s &#39;path&#39; parameter documented type is now ``py.path.local``


- `5188 &lt;https://github.com/pytest-dev/pytest/issues/5188&gt;`_: Improve help for ``--runxfail`` flag.



Trivial/Internal Changes
------------------------

- `5182 &lt;https://github.com/pytest-dev/pytest/issues/5182&gt;`_: Removed internal and unused ``_pytest.deprecated.MARK_INFO_ATTRIBUTE``.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>

bors bot added a commit to mozilla/normandy that referenced this issue May 17, 2019

Merge #1887
1887: Scheduled weekly dependency update for week 19 r=rehandalal a=pyup-bot






### Update [botocore](https://pypi.org/project/botocore) from **1.12.142** to **1.12.146**.


<details>
  <summary>Changelog</summary>
  
  
   ### 1.12.146
   ```
   ========

* api-change:``glue``: Update glue client to latest version
* api-change:``sts``: Update sts client to latest version
   ```
   
  
  
   ### 1.12.145
   ```
   ========

* api-change:``servicecatalog``: Update servicecatalog client to latest version
* api-change:``eks``: Update eks client to latest version
* api-change:``sagemaker``: Update sagemaker client to latest version
* api-change:``kinesisanalytics``: Update kinesisanalytics client to latest version
* api-change:``kinesisanalyticsv2``: Update kinesisanalyticsv2 client to latest version
   ```
   
  
  
   ### 1.12.144
   ```
   ========

* api-change:``appsync``: Update appsync client to latest version
* api-change:``storagegateway``: Update storagegateway client to latest version
* api-change:``ssm``: Update ssm client to latest version
* api-change:``alexaforbusiness``: Update alexaforbusiness client to latest version
   ```
   
  
  
   ### 1.12.143
   ```
   ========

* api-change:``config``: Update config client to latest version
* api-change:``iam``: Update iam client to latest version
* api-change:``sts``: Update sts client to latest version
* api-change:``codepipeline``: Update codepipeline client to latest version
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/botocore
  - Changelog: https://pyup.io/changelogs/botocore/
  - Repo: https://github.com/boto/botocore
</details>





### Update [Faker](https://pypi.org/project/Faker) from **1.0.5** to **1.0.6**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/faker
  - Changelog: https://pyup.io/changelogs/faker/
  - Repo: https://github.com/joke2k/faker
</details>





### Update [pluggy](https://pypi.org/project/pluggy) from **0.9.0** to **0.11.0**.


<details>
  <summary>Changelog</summary>
  
  
   ### 0.11.0
   ```
   ==========================

Bug Fixes
---------

- `205 &lt;https://github.com/pytest-dev/pluggy/issues/205&gt;`_: Revert changes made in 0.10.0 release breaking ``.egg`` installs.
   ```
   
  
  
   ### 0.10.0
   ```
   ==========================

Features
--------

- `199 &lt;https://github.com/pytest-dev/pluggy/issues/199&gt;`_: Switch from ``pkg_resources`` to ``importlib-metadata`` for entrypoint detection for improved performance and import time.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pluggy
  - Changelog: https://pyup.io/changelogs/pluggy/
  - Repo: https://github.com/pytest-dev/pluggy
</details>





### Update [Pygments](https://pypi.org/project/Pygments) from **2.3.1** to **2.4.0**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pygments
  - Homepage: http://pygments.org/
</details>





### Update [pyrsistent](https://pypi.org/project/pyrsistent) from **0.15.1** to **0.15.2**.


<details>
  <summary>Changelog</summary>
  
  
   ### 0.15.2
   ```
   * Fix 166, Propagate &#39;ignore_extra&#39; param in hierarchy. Thanks ss18 for this!
 * Fix 167, thaw typing. Thanks nattofriends for this!
 * Fix 154, not possible to insert empty pmap as leaf node with transform.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pyrsistent
  - Changelog: https://pyup.io/changelogs/pyrsistent/
  - Repo: http://github.com/tobgu/pyrsistent/
</details>





### Update [boto3](https://pypi.org/project/boto3) from **1.9.142** to **1.9.146**.


<details>
  <summary>Changelog</summary>
  
  
   ### 1.9.146
   ```
   =======

* api-change:``glue``: [``botocore``] Update glue client to latest version
* api-change:``sts``: [``botocore``] Update sts client to latest version
   ```
   
  
  
   ### 1.9.145
   ```
   =======

* api-change:``servicecatalog``: [``botocore``] Update servicecatalog client to latest version
* api-change:``eks``: [``botocore``] Update eks client to latest version
* api-change:``sagemaker``: [``botocore``] Update sagemaker client to latest version
* api-change:``kinesisanalytics``: [``botocore``] Update kinesisanalytics client to latest version
* api-change:``kinesisanalyticsv2``: [``botocore``] Update kinesisanalyticsv2 client to latest version
   ```
   
  
  
   ### 1.9.144
   ```
   =======

* api-change:``appsync``: [``botocore``] Update appsync client to latest version
* api-change:``storagegateway``: [``botocore``] Update storagegateway client to latest version
* api-change:``ssm``: [``botocore``] Update ssm client to latest version
* api-change:``alexaforbusiness``: [``botocore``] Update alexaforbusiness client to latest version
   ```
   
  
  
   ### 1.9.143
   ```
   =======

* api-change:``config``: [``botocore``] Update config client to latest version
* api-change:``iam``: [``botocore``] Update iam client to latest version
* api-change:``sts``: [``botocore``] Update sts client to latest version
* api-change:``codepipeline``: [``botocore``] Update codepipeline client to latest version
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/boto3
  - Changelog: https://pyup.io/changelogs/boto3/
  - Repo: https://github.com/boto/boto3
</details>





### Update [django-cors-headers](https://pypi.org/project/django-cors-headers) from **2.5.3** to **3.0.1**.


<details>
  <summary>Changelog</summary>
  
  
   ### 3.0.0
   ```
   ------------------

* ``CORS_ORIGIN_WHITELIST`` now requires URI schemes, and optionally ports.
  This is part of the CORS specification
  (`Section 3.2 &lt;https://tools.ietf.org/html/rfc6454section-3.2&gt;`_) that was
  not implemented in this library, except from with the
  ``CORS_ORIGIN_REGEX_WHITELIST`` setting. It fixes a security issue where the
  CORS middleware would allow requests between schemes, for example from
  insecure ``http://`` Origins to a secure ``https://`` site.

  You will need to update your whitelist to include schemes, for example from
  this:

  .. code-block:: python

      CORS_ORIGIN_WHITELIST = [&#39;example.com&#39;]

  ...to this:

  .. code-block:: python

      CORS_ORIGIN_WHITELIST = [&#39;https://example.com&#39;]

* Removed the ``CORS_MODEL`` setting, and associated class. It seems very few,
  or no users were using it, since there were no bug reports since its move to
  abstract in version 2.0.0 (2017-01-07). If you *are* using this
  functionality, you can continue by changing your model to not inherit from
  the abstract one, and add a signal handler for ``check_request_enabled`` that
  reads from your model. Note you&#39;ll need to handle the move to include schemes
  for Origins.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/django-cors-headers
  - Changelog: https://pyup.io/changelogs/django-cors-headers/
  - Repo: https://github.com/ottoyiu/django-cors-headers
</details>





### Update [djangorestframework](https://pypi.org/project/djangorestframework) from **3.9.3** to **3.9.4**.


*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/djangorestframework
  - Changelog: https://pyup.io/changelogs/djangorestframework/
  - Homepage: https://www.django-rest-framework.org/
</details>





### Update [factory_boy](https://pypi.org/project/factory_boy) from **2.11.1** to **2.12.0**.


<details>
  <summary>Changelog</summary>
  
  
   ### 2.12.0
   ```
   -------------------

*New:*

    - Add support for Python 3.7
    - Add support for Django 2.1
    - Add :attr:`~factory.fuzzy.FuzzyChoice.getter` to :class:`~factory.fuzzy.FuzzyChoice` that mimics
      the behavior of ``getter`` in :class:`~factory.Iterator`
    - Make the ``extra_kwargs`` parameter of :meth:`~factory.faker.Faker.generate` optional
    - Add :class:`~factory.RelatedFactoryList` class for one-to-many support, thanks `Sean Harrington &lt;https://github.com/seanharr11&gt;`_.
    - Make the `locale` argument for :class:`~factory.faker.Faker` keyword-only

*Bugfix:*

    - Allow renamed arguments to be optional, thanks to `Justin Crown &lt;https://github.com/mrname&gt;`_.
    - Fix `django_get_or_create` behavior when using multiple fields with `unique=True`, thanks to `YPCrumble &lt;https://github.com/YPCrumble&gt;`
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/factory-boy
  - Changelog: https://pyup.io/changelogs/factory-boy/
  - Repo: https://github.com/FactoryBoy/factory_boy
</details>





### Update [kinto-http](https://pypi.org/project/kinto-http) from **10.3.0** to **10.4.0**.


<details>
  <summary>Changelog</summary>
  
  
   ### 10.4.0
   ```
   ===================

- Add support for Bearer tokens in the CLI utilities.
- Use black for code formatting.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/kinto-http
  - Changelog: https://pyup.io/changelogs/kinto-http/
  - Repo: https://github.com/Kinto/kinto-http.py/
</details>





### Update [pytest](https://pypi.org/project/pytest) from **4.4.1** to **4.5.0**.


<details>
  <summary>Changelog</summary>
  
  
   ### 4.4.2
   ```
   =========================

Bug Fixes
---------

- `5089 &lt;https://github.com/pytest-dev/pytest/issues/5089&gt;`_: Fix crash caused by error in ``__repr__`` function with both ``showlocals`` and verbose output enabled.


- `5139 &lt;https://github.com/pytest-dev/pytest/issues/5139&gt;`_: Eliminate core dependency on &#39;terminal&#39; plugin.


- `5229 &lt;https://github.com/pytest-dev/pytest/issues/5229&gt;`_: Require ``pluggy&gt;=0.11.0`` which reverts a dependency to ``importlib-metadata`` added in ``0.10.0``.
  The ``importlib-metadata`` package cannot be imported when installed as an egg and causes issues when relying on ``setup.py`` to install test dependencies.



Improved Documentation
----------------------

- `5171 &lt;https://github.com/pytest-dev/pytest/issues/5171&gt;`_: Doc: ``pytest_ignore_collect``, ``pytest_collect_directory``, ``pytest_collect_file`` and ``pytest_pycollect_makemodule`` hooks&#39;s &#39;path&#39; parameter documented type is now ``py.path.local``


- `5188 &lt;https://github.com/pytest-dev/pytest/issues/5188&gt;`_: Improve help for ``--runxfail`` flag.



Trivial/Internal Changes
------------------------

- `5182 &lt;https://github.com/pytest-dev/pytest/issues/5182&gt;`_: Removed internal and unused ``_pytest.deprecated.MARK_INFO_ATTRIBUTE``.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>







Co-authored-by: pyup-bot <github-bot@pyup.io>
Co-authored-by: Michael Cooper <mythmon@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.