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

fixed formatting and clean up for tables #98

Merged
merged 3 commits into from Jun 19, 2017

Conversation

Projects
None yet
5 participants
@shibasisp
Contributor

shibasisp commented Jun 14, 2017

No description provided.

@shibasisp shibasisp referenced this pull request Jun 14, 2017

Closed

Added tests for tables #97

@shibasisp shibasisp force-pushed the shibasisp:table branch from 3dd3ce4 to 17fafd2 Jun 15, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 15, 2017

Coverage Status

Coverage increased (+0.1%) to 54.045% when pulling 17fafd2 on shibasisp:table into 00fb70f on casacore:master.

@@ -1796,19 +1795,18 @@ def browse(self, wait=True, tempname="/tmp/seltable"):
os.system('casabrowser ' + self.name() + waitstr1)
elif len(tempname) > 0:
six.print_(" making a persistent copy in table " + tempname)
tx = self.copy(tempname)
tx = 0

This comment has been minimized.

@tammojan

tammojan Jun 15, 2017

Contributor

Are you sure those lines can go?

This comment has been minimized.

@shibasisp

shibasisp Jun 16, 2017

Contributor

tx is a local variable and I don't see anywhere else in the method, it is being used. Moreover, tx is assigned 0 immediately after it has been assigned self.copy(tempname) which makes the assignment of self.copy(tempname) irrelevant .Its value won't change dynamically, so,I removed it.

This comment has been minimized.

@tammojan

tammojan Jun 16, 2017

Contributor

self.copy has side effects (namely it creates a new file). So please do not remove the line. I think replacing these two lines with just
self.copy(tempname) would be ok, since indeed the return value is not used.

This comment has been minimized.

@gervandiepen

gervandiepen Jun 19, 2017

Contributor

I think the changes are fine and can be merged. Tammo, do you do that?

@coveralls

This comment has been minimized.

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.09%) to 54.0% when pulling cd53f20 on shibasisp:table into 00fb70f on casacore:master.

@tammojan tammojan merged commit d6834d5 into casacore:master Jun 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 54.0%
Details

@gijzelaerr gijzelaerr added this to the 2.2 milestone Nov 6, 2017

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