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

PEP8 changes #1021

Merged
merged 6 commits into from
Oct 9, 2016
Merged

Conversation

kain88-de
Copy link
Member

Changes made in this Pull Request:

  • just clean up most complains of my PEP8 checker (flake8)

I'm sure this fixed some bugs due to missing imports. I haven't written tests for that code yet though.

PR Checklist

  • Tests?

just clean up most complains of my PEP8 checker (flake8)
@@ -907,7 +913,7 @@ def forces(self):
def forces(self, values):
ts = self._u.trajectory.ts
try:
ts.forces[self._ix, :] = forces
ts.forces[self._ix, :] = values
Copy link
Member

Choose a reason for hiding this comment

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

This should have tripped a test....

Copy link
Member Author

Choose a reason for hiding this comment

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

one would think so. I'm currently writing tests for the write function. If you find more spots where tests are needed to be triggered by this change please mark them

Copy link
Member Author

Choose a reason for hiding this comment

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

where would old tests for this be?

@@ -2,7 +2,8 @@
# vim: tabstop=4 expandtab shiftwidth=4 softtabstop=4 fileencoding=utf-8
#
# MDAnalysis --- http://www.MDAnalysis.org
# Copyright (c) 2006-2015 Naveen Michaud-Agrawal, Elizabeth J. Denning, Oliver Beckstein
# Copyright (c) 2006-2015 Naveen Michaud-Agrawal, Elizabeth J. Denning, Oliver
Copy link
Member

Choose a reason for hiding this comment

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

If you change this here, will @tylerjereddy 's #1002 still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. I can remove this one change though. But we have this mixed format in almost all the files already. Everytime I changed the formatting in some random file.

@@ -20,11 +21,12 @@
import numpy as np
import functools
import itertools
import os
Copy link
Member

Choose a reason for hiding this comment

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

Really, os was used but not imported and did not fail a test?

Copy link
Member Author

Choose a reason for hiding this comment

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

No the same goes for transformations. I'm writing tests now that are affected by the changes. But this just shows how important a good style checker is.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is tested now.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is fixed now.

@orbeckst
Copy link
Member

orbeckst commented Oct 7, 2016

Ok, then Tyler will have to deal with it.

On 7 Oct, 2016, at 12:20, Max Linke [email protected] wrote:

No idea. I can remove this one change though. But we have this mixed format in almost all the files already. Everytime I changed the formatting in some random file.

Oliver Beckstein * [email protected]
skype: orbeckst * [email protected]

@kain88-de
Copy link
Member Author

@richardjgowers I'd appreciate some help with the failing travis tests. Not sure if I accidentally broke something with this.

@richardjgowers
Copy link
Member

Those 4 failures are OK, they got added with some recent new tests Kyle added

- make code simpler
- no filename given means pdbs are written
Copy link
Member Author

@kain88-de kain88-de left a comment

Choose a reason for hiding this comment

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

where would tests for that be. I'm still a bit confused by the new github UI as it looks.

@@ -907,7 +913,7 @@ def forces(self):
def forces(self, values):
ts = self._u.trajectory.ts
try:
ts.forces[self._ix, :] = forces
ts.forces[self._ix, :] = values
Copy link
Member Author

Choose a reason for hiding this comment

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

where would old tests for this be?

@kain88-de kain88-de force-pushed the issue-363-pep8change branch from d287ed8 to dcc1a84 Compare October 7, 2016 20:12
@kain88-de
Copy link
Member Author

OK after looking into develop the transformations tests have never existed. So I will take care of them with #1010. @richardjgowers if you want to take care of the force setter tests I would consider this done.

@richardjgowers
Copy link
Member

Force setter tests I think are in something called
test_velocities_forces.py

On Fri, 7 Oct 2016, 9:16 p.m. Max Linke, [email protected] wrote:

OK after looking into develop the transformations tests have never
existed. So I will take care of them with #1010
#1010. @richardjgowers
https://github.com/richardjgowers if you want to take care of the force
setter tests I would consider this done.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1021 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AI0jBzsBgrQLcHamcvMygM_B2UQmw5h5ks5qxqiKgaJpZM4KRWh3
.

@kain88-de
Copy link
Member Author

I've also added tests for the forces now.

@richardjgowers richardjgowers merged commit 4370b74 into MDAnalysis:issue-363 Oct 9, 2016
@kain88-de kain88-de deleted the issue-363-pep8change branch October 12, 2016 14:23
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.

3 participants