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

add missing __slots__ to containers and their base classes (#1144) #1147

Merged
merged 2 commits into from
Nov 28, 2021
Merged

add missing __slots__ to containers and their base classes (#1144) #1147

merged 2 commits into from
Nov 28, 2021

Conversation

ariebovenberg
Copy link
Contributor

@ariebovenberg ariebovenberg commented Nov 27, 2021

I have made things!

  • slots added to many many classes
  • in pytest plugin, the mechanism for tracking 'errors handled' needed a rework
  • test added to prevent forgetting slots in the future

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Open questions

In adding __slots__, internal changes were needed to the pytest plugin. It works now (as discussed in #1144), but I still have a nagging feeling (monkey-patching and mutable global state will do that... 😉 )

I would like to suggest a follow-up change to the pytest plugin: that it undoes its patches after each test (like unittest.mock.patch does). The advantages are:

  • classes are only patched when using the special ReturnsAsserts. This means it 100% doesn't affect any other tests (i.e. we get more predictable behavior). Currently all tests running after a returns-enabled test are patched, those before are not. A very nasty thing to debug if it ever messes with anybody's tests 👀
  • the 'errors handled' mapping doesn't need to be a mutable global variable (👿) , it can be stored in the ReturnsAsserts instance.
  • the 'errors handled' mapping doesn't need to be cleared after each test 🚀

🤔 Of course, we'll have to see what the performance impact is. Perhaps we might need to split the performance-heavy patching into a separate fixture, so it's only activated when needed

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@sobolevn
Copy link
Member

classes are only patched when using the special ReturnsAsserts. This means it 100% doesn't affect any other tests (i.e. we get more predictable behavior). Currently all tests running after a returns-enabled test are patched, those before are not. A very nasty thing to debug if it ever messes with anybody's tests

Agreed!

the 'errors handled' mapping doesn't need to be a mutable global variable (👿) , it can be stored in the ReturnsAsserts instance.

Yes, I thought so too during code review.

@ariebovenberg
Copy link
Contributor Author

I'll create a follow-up issue for the pytest plugin changes

There seems to be an issue with flake8 failing the the build, but not locally. Investigating...

@ariebovenberg
Copy link
Contributor Author

It seems that for different python versions, flake8 attributes WPS602 errors to different lines of code. Double noqas were needed in pytest plugin. @sobolevn you can probably tell if this is a problem with the style guide, or flake8?

# passes in py37, but not py38+
class A:
    @staticmethod  # noqa: WPS602
    def foo():
        ...

# passes in py38+, but not py37
class A:
    @staticmethod
    def foo():    # noqa: WPS602
        ...

# passes on all versions:
class A:
    @staticmethod  # noqa: WPS602
    def foo():  # noqa: WPS602
        ...

@codecov
Copy link

codecov bot commented Nov 28, 2021

Codecov Report

Merging #1147 (13cbf95) into master (51f91c7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1147   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           79        79           
  Lines         2338      2415   +77     
  Branches       153       153           
=========================================
+ Hits          2338      2415   +77     
Impacted Files Coverage Δ
returns/context/requires_context.py 100.00% <100.00%> (ø)
returns/context/requires_context_future_result.py 100.00% <100.00%> (ø)
returns/context/requires_context_ioresult.py 100.00% <100.00%> (ø)
returns/context/requires_context_result.py 100.00% <100.00%> (ø)
returns/future.py 100.00% <100.00%> (ø)
returns/interfaces/altable.py 100.00% <100.00%> (ø)
returns/interfaces/applicative.py 100.00% <100.00%> (ø)
returns/interfaces/bimappable.py 100.00% <100.00%> (ø)
returns/interfaces/bindable.py 100.00% <100.00%> (ø)
returns/interfaces/container.py 100.00% <100.00%> (ø)
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51f91c7...13cbf95. Read the comment docs.

@ariebovenberg ariebovenberg marked this pull request as ready for review November 28, 2021 09:52
@sobolevn
Copy link
Member

@ariebovenberg yes, this is true 😞

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

I agree with your WPS issue, that things like __bases__, __module__, etc should be allowed.

Thanks a lot for working on this! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

__slots__ = () missing in many base classes?
2 participants