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.

Sign up for GitHub

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

Jump to bottom

Add pep8 master #1303

Merged
merged 4 commits into from
May 14, 2015
Merged

Add pep8 master #1303

merged 4 commits into from
May 14, 2015

Conversation

aashish24
Copy link
Contributor

No description provided.

doutriaux1 and others added 4 commits May 9, 2015 09:02
@aashish24
Copy link
Contributor Author

Refs: #1290.

  • This branch should be for master only
  • We should build pep8 only for developers and developers should have build testing enabled.

doutriaux1
doutriaux1 reviewed May 11, 2015
View reviewed changes
CMake/cdat_modules/pep8_pkg.cmake
set(${uc_nm}_VERSION ${${nm}_MAJOR}.${${nm}_MINOR}.${${nm}_PATCH})
set(PEP8_SOURCE ${PEP8_URL}/${PEP8_GZ})

if (BUILD_TESTING)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we turn on build testing? is it on by default?

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 is on by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can turn it OFF by default but I think its mostly us (developers) building it for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we turn it off, does that means ctest will not work? If so, leaving it ON is probably best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, there won't be any ctesting if it is OFF. I agree with you having testing ON by default is good. Having PEP installed when testing is ON is good IMO. If you agree, we can have it merged and then @dlonie can add style testing.

@aashish24
Copy link
Contributor Author

I am merging this one since this wasn't really my branch and I approved it

aashish24 added a commit that referenced this pull request May 14, 2015
@aashish24
Merge pull request #1303 from UV-CDAT/add_pep8_master
3ab2222 
Add pep8 master
@aashish24 aashish24 merged commit 3ab2222 into master May 14, 2015
@aashish24 aashish24 deleted the add_pep8_master branch May 14, 2015 00:25
@aashish24 aashish24 mentioned this pull request May 22, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aashish24 @doutriaux1