Add pep8

[doutriaux1]

@aashish24 @dlonie @remram44 @chaosphere2112 correct branch this time.

[remram44]

I'm not sure there's a point in this, it is a development tool that the user will already have if he's using it. And activating the UV-CDAT environment probably doesn't interfere with it.

Sticking to a particular code style for UV-CDAT would probably be useful though.

[doutriaux1]

but it doesn't hurt to have it. And for example our users here (and at other universities) only use UV-CDAT's Python so I'm trying to get them a complete set of tools. It's tiny too.

[aashish24]

it should be OFF even in ALL mode. ALL is meant for users and PEP is a developer tool. In near future, I would like to test ALL as well and more you will add to ALL, it will make it difficult to test it on dashboard machines.

I like the idea and wanted to talk about it.

[remram44]

Since pip is available, users are a lot more likely to "pip install" it than remember to activate it before building, if it's not enabled by default. Just my two cents of course, I have no strong opinion about any of this.

[doutriaux1]

we can't use pip at the lab... firewall blocking us... @aashish24 ok I like to have it on only in full build.

[aashish24]

Nice to have it +1 other than my request it looks good.

[aashish24]

Sure. I am thinking continuous testing / building point of view. Anytime we add a package:

1. We increase the chances that it will fail on some systems
2. Build time increases.

Maybe we should change the name ALL to something else (SUPER) and then have ALL build all of the packages. SUPER or something like that makes sense since we have LEAN and DEFAULT now.

[doutriaux1]

@aashish24 let's think about it. It gets complicated we have LEAN/DEFAULT/ALL the more add the more confusing it gets.

[alliepiper]

I'm curious about the point of this, too. Just to have it available? I'm not sure developer tools really belong in the superbuild.

Now, if you wanted to use this (and ideally, a linter) to add tests that check the source code for consistency and correctness (and were willing to clean up the existing issues these tools would uncover), then I'd be 100% behind it...

I won't object to it going in, but if it does IMO it should be disabled by default for all builds, and only built when explicitly requested in cmake. The superbuild is already quite bloated.

[alliepiper]

Actually, if it goes in, make it on by default and I'll add a unit test that tests the new vcs.vcsvtk submodule. I'd love to have patches with poor readability rejected by a test for that submodule. Hopefully others would follow suit and do the same for their code.

[aashish24]

@dlonie I liked the idea. For the testing and dashboards, we can setup the cmake to turn it ON. I would suggest we leave it OFF in all cases and tweak config for dashboards to turn it ON. What you all think?

@doutriaux1 I agree 3 options are good. Adding 4th could be confusing. I was just proposing a solution but wasn't strongly favoring it.

[alliepiper]

How about it gets turned on anytime testing is enabled? I'd like any dev running tests to see issues before the pull request gets made, without having to manually set an option.

Re: build tools in the superbuild, I just noticed that spyder is in there. Anyone else think a user's preferred IDE should be up to them to maintain on their own machine, rather than lumping it in with the superbuild?

[durack1]

@dlonie @aashish24 @doutriaux1 I wonder if code coverage is something that should also be considered? #922 suggested this..

[alliepiper]

Improving test coverage would certainly improve the quality/reliability of the code. I'm not familiar with how coverage is tested in python, so I'm not sure how much effort it would take, though.

[aashish24]

+100000000 for coverage. I was about to work on it. @jbeezley has done some work. I will talk to him this afternoon. Yes, we need code coverage. Earlier, this week, I thought about making it as a first priority since only then we know exactly how much code we are testing.

[chaosphere2112]

@dlonie @durack1 I've been working on getting some test coverage numbers with the tests I've been writing for the vcs.vtk_ui submodule. There's a module called coverage.py that I've integrated with my tests (though all of my tests follow a convention, and derive from the same base class). I'm currently working on pushing my coverage up above 90%– sitting at 62% right now on a branch.

[aashish24]

@dlonie How about it gets turned on anytime testing is enabled? I

+1. Do you want to push that change? I liked that idea.

• Aashish

[aashish24]

@chaosphere2112 yes that's what @jbeezley uses as well: https://github.com/OpenGeoscience/gaia/blob/master/CMakeLists.txt

[chaosphere2112]

@aashish24 @jbeezley Here's the script I've been using to get some coverage numbers; at one point I modified it to get numbers for the bulk of the VCS tests, but it did involve some substantial editing of the existing tests, so I just did it as an exploration.

[alliepiper]

@aashish24 I won't have time to today, but I can look into that next week if no one beats me to it.

[remram44]

The coverage tool is great and pretty straightforward (it does slow down the code, though). Free service coveralls.io can display coverage status and alert/comment on PRs, works great with Travis (see here for example)

[chaosphere2112]

@remram44 Yup, that's what we're all looking at 👍

[alliepiper]

Well, we don't need a tool for our travis coverage -- it's '0'.

Just noticed yesterday that it doesn't actually run any tests...

[chaosphere2112]

@dlonie I think that was part of the shift over to buildbot; the tests were almost always failing on travis

[durack1]

It seems you're spoilt for choice with this, https://github.com/z4r/python-coveralls and https://github.com/coagulant/coveralls-python are two other options listed off https://coveralls.zendesk.com/hc/en-us/articles/201342869-Python

[jbeezley]

Coveralls works with the coverage.py output files. It is easy to get working. As for coverage.py, it is easiest to just run your tests with the coverage script.

python -m unittest ... ==> coverage -m unittest ...

or

python testscript.py ... ==> coverage testscript.py ...

The only difficulty is getting coverage to execute the correct python binary, use the correct python path, etc.

[alliepiper]

Sounds like the new buildbot runtest script could take care of calling the coverage tool in the correct environment for us.

[chaosphere2112]

@dlonie That does sound easier than integrating it into the test process for every developer; would probably encourage people like me (who like to drop the massive PRs 😉) to submit earlier, so we can get coverage numbers while we're in-progress.

[aashish24]

right. the path should not be a problem for us anymore. Also,

• we cannot run tests on travis ci because of time limitation and various other issues
• we moved to use buildbot for that purpose. travis is used only to test builds
• I can add code coverage stuff with help from @jbeezley and @chaosphere2112
• @remram44 coverage.io looks good. I have see that before as I think you are using it for reprozip

[remram44]

@aashish24 The ReproZip situation is a bit weird since I need coveralls to handle both C code and Python code coverage, which come from two different Travis jobs... It's in need of fixing unfortunately.

It did work at one point, though.

[aashish24]

thanks @remram44.

[aashish24]

@doutriaux1 this branch should go into master. I don't think its needed in release.

[aashish24]

I am closing this pull request and will create a new one based on master.