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

Code maxx 138 fix #678

Merged
merged 13 commits into from Dec 26, 2015

Conversation

Projects
None yet
4 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Dec 11, 2015

Trailing Whitespace detection utility(Issue#138)

@certik

This comment has been minimized.

Copy link
Contributor

certik commented Dec 11, 2015

Thanks! It should also test all the .h files. And then be integrated into our tests to run on Travis once.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 11, 2015

Added test for .h files.

@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented Dec 12, 2015

Name your Pull Requests better, not an issue here but a good practice.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 13, 2015

I'll take care of that next time. Will this be merged or does this require improvement yet?

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 13, 2015

Can you run this check in one Travis test?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 13, 2015

It passed the Travis and Appveyor check. Do I need to manually do any other test?

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 13, 2015

@CodeMaxx, what I meant was to run this check in travis, so that a PR author will be notified of trailing whitespaces from travis.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 13, 2015

@isuruf , I'm new to this so I don't exactly know how to do that .Could you please help out a bit

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 13, 2015

Sure.
First how do you run this check?
python utilities/test/test_trailing_whitespace.py from the root?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 13, 2015

yes.

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 13, 2015

Add this to the end of bin/test_travis.sh

if [[ "${TEST_TRAILING_WS}" == "yes" ]]; then
    python utilities/test/test_trailing_whitespace.py
fi

what this does is run the test when the variable TEST_TRAILING_WS is set to yes.

To set that variable change the line https://github.com/symengine/symengine/blob/master/.travis.yml#L29
to

  - BUILD_TYPE="Debug" WITH_BFD="yes" TEST_TRAILING_WS="yes"
.travis.yml Outdated
@@ -26,7 +26,7 @@ env:

## Out of tree builds (default):
# Debug build (with BFD)
- BUILD_TYPE="Debug" WITH_BFD="yes"
- BUILD_TYPE="Debug" WITH_BFD="yes" TEST_TRAILING_WS="yes"

This comment has been minimized.

@isuruf

isuruf Dec 13, 2015

Member

Remove the extra spaces at the beginning

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 13, 2015

The test didn't pass. Says 'file not found'

@@ -92,3 +92,6 @@ if [[ "${TEST_CPP}" != "no" ]]; then
./a.out
fi

if [[ "${TEST_TRAILING_WS}" == "yes" ]]; then
python utilities/test/test_trailing_whitespace.py

This comment has been minimized.

@isuruf

isuruf Dec 13, 2015

Member

Try adding the following before this line.
cd ${SOURCE_DIR}

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 14, 2015

Travis passed!

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 14, 2015

There are some more things to do

  • Skip build directory
  • Make travis test fail if there are trailing white-spaces. (Atm, there are trailing white-spaces, but tests pass)
  • Fix all the existing trailing whitespace issues
@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 14, 2015

is 'cmake' the build directory? And how to make travis fail in case of trailing whitespace?

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 21, 2015

There is a directory named build in the project root in travis, you need to skip that directory.
To make tests fail in case of trailing whitespace, throw an exception from the python script.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 25, 2015

Changed the printing format so as it looks a bit cleaner. Now it checks for .cpp and .h files in the bin,symengine and benchmarks directory and none others.
Should I skip the bin directory also since it contains no .cpp and .h files?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Dec 25, 2015

The first commit was not proper. Now it skips the mentioned directories.


def test(fname):

with open(fname, "rt") as test_file:

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

4 spaces before with instead of a tab

This comment has been minimized.

@CodeMaxx

CodeMaxx Dec 25, 2015

Contributor

but why so?

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

It's a matter of code style. SymEngine uses 4 spaces instead of tabs. See https://github.com/symengine/symengine/blob/master/doc/style_guide.md#whitespace

This comment has been minimized.

@CodeMaxx

CodeMaxx Dec 25, 2015

Contributor

So I have to change this everywhere or just before with

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

Everywhere in this file

This comment has been minimized.

@CodeMaxx

CodeMaxx Dec 25, 2015

Contributor

Done

test_this_file(fname, test_file)

with open(fname, "rt") as test_file:
source = test_file.read()

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

Same as above

if _failed_expectations:
(filename, line, funcname) = inspect.stack()[1][1:4]
report = [
'Failed Expectations:%s\n' % len(_failed_expectations)]

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

What do you mean by a 'Failed Expectation' ?

This comment has been minimized.

@CodeMaxx

CodeMaxx Dec 25, 2015

Contributor

The code is expected to have no trailing whitespace. Each trailing whitespace is a failed expectation

check_directory_tree(BIN_PATH, test, set(["~",".sh"]), "*")
check_directory_tree(SYMENGINE_PATH, test, set(["/build/","/doc/","/cmake/","/utilities"]))
check_directory_tree(SYMENGINE_PATH, test, set(["/build/","/doc/","/cmake/","/utilities"]), "*.h")
print _report_failures()

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

You should exit with an error code of 1 if there are any failures

This comment has been minimized.

@CodeMaxx

CodeMaxx Dec 25, 2015

Contributor

In the latest commit I raised an exception in case there are any failures.

report.append('%d: %s' % (i, failure))
_failed_expectations = []
if len(report) != 0:
return ('\n'.join(report))

This comment has been minimized.

@isuruf

isuruf Dec 25, 2015

Member

Use 4 spaces

This comment has been minimized.

@CodeMaxx

CodeMaxx Dec 25, 2015

Contributor

done

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Dec 25, 2015

+1
@certik, what do you think about this file location? utilities/test/test_trailing_whitespace.py

@certik

This comment has been minimized.

Copy link
Contributor

certik commented Dec 26, 2015

I think this looks good. Thanks @CodeMaxx for the work. Thanks @isuruf and @Sumith1896 for review.

certik added a commit that referenced this pull request Dec 26, 2015

Merge pull request #678 from CodeMaxx/CodeMaxx-138-fix
Add trailing whitespace tests

@certik certik merged commit 06dfd69 into symengine:master Dec 26, 2015

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

CodeMaxx added some commits Dec 26, 2015

@CodeMaxx CodeMaxx referenced this pull request Dec 31, 2015

Merged

Latest gcc Travis checks #727

@CodeMaxx CodeMaxx deleted the CodeMaxx:CodeMaxx-138-fix branch Jan 4, 2016

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