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

script help formatting #1268

Merged
merged 12 commits into from
Aug 26, 2015
Merged

script help formatting #1268

merged 12 commits into from
Aug 26, 2015

Conversation

mr-c
Copy link
Contributor

@mr-c mr-c commented Aug 18, 2015

From a fairly thorough review of the help output of all the scripts.

Has the fix for #1257 pulled from #1255

@mr-c
Copy link
Contributor Author

mr-c commented Aug 18, 2015

  • Is it mergeable?
  • Did it pass the tests?
  • If it introduces new functionality in scripts/ is it tested?
    Check for code coverage with make clean diff-cover
  • Is it well formatted? Look at make pep8, make diff_pylint_report,
    make cppcheck, and make doc output. Use make format and manual
    fixing as needed.
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Is it documented in the ChangeLog?
    http://en.wikipedia.org/wiki/Changelog#Format
  • Was a spellchecker run on the source code and documentation after
    changes were made?
  • Do the changes respect streaming IO? (Are they
    tested for streaming IO?)
  • Is the Copyright year up to date?

@mr-c
Copy link
Contributor Author

mr-c commented Aug 18, 2015

@ctb @camillescott ready for review and merge

@mr-c mr-c force-pushed the fix/script-help-formatting branch from e2ae53d to bb06c26 Compare August 20, 2015 01:34
@mr-c
Copy link
Contributor Author

mr-c commented Aug 20, 2015

Rebased on master and again ready for review & merge @camillescott ; thanks!

@mr-c mr-c added this to the 2.0 milestone Aug 22, 2015
@mr-c mr-c force-pushed the fix/script-help-formatting branch from bb06c26 to 0cd59f8 Compare August 25, 2015 03:15
@mr-c
Copy link
Contributor Author

mr-c commented Aug 25, 2015

Jenkins, retest this please

@mr-c
Copy link
Contributor Author

mr-c commented Aug 25, 2015

Again made ready for review & merge @camillescott @ctb

Note: I've disabled the 'oxli' script for the v2.0 release. I will re-enable it right after the release is finished.

@ctb
Copy link
Member

ctb commented Aug 25, 2015

On Mon, Aug 24, 2015 at 10:17:53PM -0700, Michael R. Crusoe wrote:

Note: I've disabled the 'oxli' script for the v2.0 release. I will re-enable it right after the release is finished.

Why?

@mr-c
Copy link
Contributor Author

mr-c commented Aug 25, 2015

@ctb It isn't ready for non-devs and it currently gets installed very visibly. No need to confuse anyone. One simple PR will return it for further development after 2.0 is released.

SensibleSalmon
SensibleSalmon reviewed Aug 25, 2015
View reviewed changes
scripts/do-partition.py
@@ -41,10 +41,10 @@
DEFAULT_K = 32


def worker(queue, basename, stop_big_traversals):
def worker(que, basename, stop_big_traversals):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes #1257
queue was hiding the module queue

@camillescott
Copy link
Member

cool, LGTM

@mr-c
Copy link
Contributor Author

mr-c commented Aug 26, 2015

Thanks! Merge?

camillescott added a commit that referenced this pull request Aug 26, 2015
@camillescott
Merge pull request #1268 from dib-lab/fix/script-help-formatting
24bacd7 
script help formatting
@camillescott camillescott merged commit 24bacd7 into master Aug 26, 2015
@mr-c mr-c mentioned this pull request Aug 26, 2015
Closed
@ctb
Copy link
Member

ctb commented Aug 26, 2015

On Tue, Aug 25, 2015 at 04:06:26PM -0700, Michael R. Crusoe wrote:

@ctb It isn't ready for non-devs and it currently gets installed very visibly. No need to confuse anyone. One simple PR will return it for further development after 2.0 is released.

OK. Please let's expose these decisions with explanations in either
an issue or in the pull request...

@mr-c mr-c deleted the fix/script-help-formatting branch September 4, 2015 23:59
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
2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mr-c @ctb @camillescott @SensibleSalmon