Skip to content

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Nov 7, 2019

This is a useful utility to abstract the caching property idiom.

It is in compat.py since eventually it will be replaced by
functools.cached_property.

Fixes #6131.

location[2],
) # type: Tuple[str, Optional[int], str]
return self._location
location = self.reportinfo()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With regard to this I've noticed that we'd want to cache reportinfo() probably/maybe - any ideas how to do this? Should/could we wrap it in a property with a different name then? lru might also be an option, but seems bad with just a single item?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for methods we can implement a simple decorator that does something similar to cached_property, say cached_method. It should be even simpler than cached_property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lru might also be an option, but seems bad with just a single item?!

lru_cache is very fast in my experience (in Python terms...). However, it can't really be used on methods, it leaks self.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets leave the method caching for later for now,
it can trivially be implemented as shim for a new private property if necessary

This is a useful utility to abstract the caching property idiom.

It is in compat.py since eventually it will be replaced by
functools.cached_property.

Fixes pytest-dev#6131.
@bluetech
Copy link
Member Author

Squashed and rebased -- will merge once it passes CI.

@bluetech
Copy link
Member Author

Hmm, is adding a # pragma: no cover to the py38 branch the correct fix for the coverage CI failure? It seems wrong because it depends on the python version calculating the coverage. Or is coverage combined from multiple versions?

@blueyed
Copy link
Contributor

blueyed commented Nov 10, 2019

@bluetech
It shows that we have no coverage for py38 (https://codecov.io/gh/pytest-dev/pytest/compare/710e3c40e0e613a06eef62f58b3f7db9761ee868...42a46ea78617b8e210636bc2f9d9bf06435b60fa/diff#D2-393). So either add it to the job (https://github.com/blueyed/pytest/blob/04f27d4eb469fb9c76fd2d100564a0f7d30028df/.travis.yml#L62-L63), or ignore it.
I recommend to not ignore uncovered branches - it's ok to merge things without 100% coverage, i.e. it is not a "CI failure", but only a hint.

@bluetech
Copy link
Member Author

I recommend to not ignore uncovered branches - it's ok to merge things without 100% coverage, i.e. it is not a "CI failure", but only a hint.

Did you mean "not ignore" -> "ignore"?

In any case, I can't merge because it says "Required statuses must pass before merging". So I'll try to sort it out later.

@blueyed
Copy link
Contributor

blueyed commented Nov 10, 2019

Did you mean "not ignore" -> "ignore"?

I've meant to not put "# pragma" comments there to ignore it with the report.
I.e. just ignore / accept that something is not covered (or rather not run with coverage reporting enabled to be more specific).

@blueyed
Copy link
Contributor

blueyed commented Nov 10, 2019

I've removed the requirement on/for that check. /cc @nicoddemus

@bluetech bluetech merged commit b352e34 into pytest-dev:features Nov 10, 2019
@bluetech bluetech deleted the cached-property branch November 10, 2019 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants