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

added test for #361, indent problem in script #616

Merged
merged 1 commit into from Sep 29, 2014

Conversation

SherineAwad
Copy link
Contributor

also see #608, a contaminated pull request that we are replacing.

@SherineAwad
Copy link
Contributor Author

Ref issue #361

@SherineAwad
Copy link
Contributor Author

@mr-c could you take a look at this? I think it's good but I did the last set of pushes so it should be reviewed by someone else. TIA!

@mr-c mr-c added this to the 1.2+ milestone Sep 24, 2014
@@ -1,3 +1,6 @@
2014-09-22 Sherine Awad <sherine.awad@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Two spaces between the date and your name and your name and your email address please.

@mr-c
Copy link
Contributor

mr-c commented Sep 24, 2014

  • 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 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.
  • Is it documented in the ChangeLog?
  • Was a spellchecker run on the source code and documentation after
    changes were made?

args = ['--loadtable', hashfile, infile]
(status, out, err) = utils.runscript(script, args)
assert status == 0, (out, err)
print (out, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: The print statement isn't technically needed as the (out, err) will be presented if the assertion fails. Not a problem, though

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, this does help quite a lot with debugging given the line squashing that Nose does

@mr-c
Copy link
Contributor

mr-c commented Sep 24, 2014

I verified this by re-introducing the indent error in 3d79783 this test (and only this test) does indeed catch it.

LGTM. Merge once the ChangeLog formatting is fixed.

@SherineAwad SherineAwad merged commit 4e0b6c6 into master Sep 29, 2014
@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

2 participants