-
Notifications
You must be signed in to change notification settings - Fork 736
Pytest Style: core/test_atom.py #1535
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
Conversation
| def test_attributes_positions(self, atom): | ||
| a = atom | ||
| # new position property (mutable) | ||
| assert_almost_equal(a.position, self.known_pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
known_pos is used once within the class, it can just live in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
jbarnoud
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very little to change.
| def test_no_velo(self, atom): | ||
| def lookup_velo(): | ||
| return self.atom.velocity | ||
| return atom.velocity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are using the context manager to test the exception, you do not need the nested function.
| def test_bad_add(self, atom): | ||
| def bad_add(): | ||
| return self.atom + 1 | ||
| return atom + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not need the nested function here.
|
@jbarnoud @richardjgowers Good to go now? |
|
@utkbansal I've restarted the travis build. I'm curious why there is a drop of 0.03 percent in coverage. Maybe it goes away after rerunning the test but it would be nice if you could have a look into this. |
|
@kain88-de 0.01% Coveralls shows nothing. |
Fixes #
Changes made in this Pull Request:
PR Checklist