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

Fix/make format target #612

Merged
merged 1 commit into from Sep 30, 2014
Merged

Conversation

brtaylor92
Copy link
Contributor

Add 'astyle' and 'format' targets to Makefile

@brtaylor92
Copy link
Contributor Author

  • Is it mergable
  • Did it pass the tests?
  • If it introduces new functionality in scripts/ is it tested?
    Check for code coverage.
  • Is it well formatted? Look at pep8/pylint, cppcheck, and
    make doc output. Use autopep8 and astyle -A10 --max-code-length=80
    if needed.
  • Is it documented in the ChangeLog?
  • Was a spellchecker run on the source code and documentation after changes
    were made?

@@ -63,8 +63,7 @@ ready for review::
- [ ] If it introduces new functionality in scripts/ is it tested?
Check for code coverage.
- [ ] Is it well formatted? Look at `make pep8`, `make diff_pylint_report`,
`make cppcheck`, and `make doc` output. Use `make autopep8`,
`astyle -A10 --max-code-length=80`, and manual fixing as needed.
`make cppcheck`, and `make doc` output. Use `make format` as needed,
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets leave the comment about manual fixing in. Autopep8 can't fix all PEP8 errors automatically

@mr-c
Copy link
Contributor

mr-c commented Sep 22, 2014

retest this please

@mr-c
Copy link
Contributor

mr-c commented Sep 22, 2014

2 nits; otherwise this looks good to me.

@mr-c mr-c added this to the 1.2+ milestone Sep 22, 2014
@brtaylor92
Copy link
Contributor Author

retest this please

1 similar comment
@mr-c
Copy link
Contributor

mr-c commented Sep 29, 2014

retest this please

@mr-c
Copy link
Contributor

mr-c commented Sep 29, 2014

Please update the pep8 version to 1.5.7 so that make install-dependencies format succeeds.

@ged-jenkins
Copy link

Test PASSed.

@mr-c
Copy link
Contributor

mr-c commented Sep 29, 2014

@brtaylor92 can you either undo the formating or merge with master?

@mr-c
Copy link
Contributor

mr-c commented Sep 29, 2014

Also your commits aren't being linked to your github account. I'm guessing the email address needs to be changed.

@ged-jenkins
Copy link

Test FAILed.

@ged-jenkins
Copy link

Test PASSed.

@mr-c
Copy link
Contributor

mr-c commented Sep 29, 2014

retest this please

@ged-jenkins
Copy link

Test PASSed.

… make format target

Run autopep8

Update pep8 to 1.5.7
@ged-jenkins
Copy link

Test PASSed.

mr-c added a commit that referenced this pull request Sep 30, 2014
@mr-c mr-c merged commit f6ecfe3 into dib-lab:master Sep 30, 2014
@mr-c
Copy link
Contributor

mr-c commented Sep 30, 2014

Thanks for the rebase.

@mr-c mr-c modified the milestones: 1.3+, 1.3 Dec 19, 2014
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