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

Implement next and iter for the Node.open deprecation wrapper #4399

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Sep 25, 2020

Fixes #4396

The return value of Node.open was wrapped in WarnWhenNotEntered in
aiida-core==1.4.0 in order to warn users that use the method without a
context manager, which will start to raise in v2.0. Unfortunately, the
raising came a little early as the wrapper does not implement the
__iter__ and __next__ methods, which can be called by clients.

An example is numpy.genfromtxt which will notice the return value of
Node.open is filelike and so will wrap it in iter. Without the
current fix, this raises a TypeError. The proper fix would be to
forward all magic methods to the wrapped filelike object, but it is not
clear how to do this.

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #4399 into develop will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4399      +/-   ##
===========================================
+ Coverage    79.22%   79.22%   +0.01%     
===========================================
  Files          475      475              
  Lines        34822    34826       +4     
===========================================
+ Hits         27583    27588       +5     
+ Misses        7239     7238       -1     
Flag Coverage Δ
#django 73.07% <100.00%> (+0.01%) ⬆️
#sqlalchemy 72.29% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/orm/nodes/node.py 81.82% <100.00%> (+0.18%) ⬆️
aiida/engine/daemon/runner.py 82.76% <0.00%> (+3.45%) ⬆️

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 0184518...1b97877. Read the comment docs.

greschd
greschd previously approved these changes Sep 25, 2020
Copy link
Member

@greschd greschd left a comment

Choose a reason for hiding this comment

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

Do we want a regression test for this?

The return value of `Node.open` was wrapped in `WarnWhenNotEntered` in
`aiida-core==1.4.0` in order to warn users that use the method without a
context manager, which will start to raise in v2.0. Unfortunately, the
raising came a little early as the wrapper does not implement the
`__iter__` and `__next__` methods, which can be called by clients.

An example is `numpy.getfromtxt` which will notice the return value of
`Node.open` is filelike and so will wrap it in `iter`. Without the
current fix, this raises a `TypeError`. The proper fix would be to
forward all magic methods to the wrapped filelike object, but it is not
clear how to do this.
@sphuber
Copy link
Contributor Author

sphuber commented Sep 25, 2020

Do we want a regression test for this?

Done

@sphuber
Copy link
Contributor Author

sphuber commented Sep 25, 2020

@greschd I added a test, this is ready for second review

Copy link
Member

@greschd greschd left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber !

@sphuber sphuber merged commit 5e1c6fd into aiidateam:develop Sep 25, 2020
@sphuber sphuber deleted the fix/4396/node-open-deprecation-wrapper branch September 25, 2020 13:57
sphuber added a commit to sphuber/aiida-core that referenced this pull request Sep 25, 2020
…iidateam#4399)

The return value of `Node.open` was wrapped in `WarnWhenNotEntered` in
`aiida-core==1.4.0` in order to warn users that use the method without a
context manager, which will start to raise in v2.0. Unfortunately, the
raising came a little early as the wrapper does not implement the
`__iter__` and `__next__` methods, which can be called by clients.

An example is `numpy.getfromtxt` which will notice the return value of
`Node.open` is filelike and so will wrap it in `iter`. Without the
current fix, this raises a `TypeError`. The proper fix would be to
forward all magic methods to the wrapped filelike object, but it is not
clear how to do this.
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.

New deprecation wrapper for Node.open causes problems with numpy
2 participants