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

Flake #96

Merged
merged 5 commits into from
Oct 24, 2017
Merged

Flake #96

merged 5 commits into from
Oct 24, 2017

Conversation

andamian
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.0%) to 66.34% when pulling 8ce3b67 on andamian:flake into b4ff77f on opencadc:master.

@andamian andamian requested a review from ijiraq October 23, 2017 21:23
Copy link
Collaborator

@ijiraq ijiraq left a comment

Choose a reason for hiding this comment

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

Not 100% on all these style choices but no errors found. I think that TestVsync class should be renamed. Not needed for this pull request.

- for i in $(ls -d */); do
cd $i;
pytest --cov $i || exit 1;
if [[ $TRAVIS_PYTHON_VERSION == '3.5' ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we stop testing against Python 3.5 at some point then this will be skipped. Is that OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The is no point in running some of these tasks for all versions of Python so I've picked one. Yes, eventually we'll have to update to a different one.

@@ -4,4 +4,4 @@
This application mounts a VOSpace service to the local file system
using fuse.
"""
from vofs import *
from vofs import * # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does 'noqa' mean? I guess this is some code flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noqa = No Quality Assurance. flake8 understandably doesn't like import *.

help="Mount the filesystem as a foreground opperation and " +
"produce copious amounts of debuging information")
parser.add_option(
"-f", "--foreground", action="store_true",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sort of line feed push is ugly and I think an auto generated thing. Note that line 30 did NOT get its arguments wrapped in this way. I suspect the issue is that action="store_true" should be on its own line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you brake the arguments, then you have to align them at the opening bracket which makes the subsequent lines quite short.

default=50 * 2 ** (10 + 10 + 10))
help="File to store debug log to",
default="/tmp/vos.err")
parser.add_option(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto to the previous comment about argument ugliness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matter of taste.


opt = parser.parse_args()
set_logging_level_from_args(opt)

log_format = ("%(asctime)s %(thread)d vos-"+str(version)+" %(module)s.%(funcName)s.%(lineno)d %(message)s")
log_format = ("%(asctime)s %(thread)d vos-" + str(
version) + " %(module)s.%(funcName)s.%(lineno)d %(message)s")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Poor choice of wrapping point.

@@ -1,4 +0,0 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this removed? Or is it? I'm confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not needed since tests is not a module.

@@ -6,7 +6,8 @@
unicode_literals)
from ..vos import Client
from ..vos import CADC_GMS_PREFIX
from ..commonparser import CommonParser, set_logging_level_from_args, exit_on_exception
from ..commonparser import CommonParser, set_logging_level_from_args, \
exit_on_exception
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would look cleaner if the from .. part was repeated.

outputs = [MyExitError] * (len(cmds.__all__) + 3)


class TestVsync(unittest.TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the wrong name for this class? TestCLI maybe?

@coveralls
Copy link

Coverage Status

Coverage increased (+2.0%) to 66.349% when pulling 477f263 on andamian:flake into b4ff77f on opencadc:master.

@andamian andamian merged commit 238ff91 into opencadc:master Oct 24, 2017
@andamian andamian deleted the flake branch October 24, 2017 19:53
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.

None yet

3 participants