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/extract long seq toscripts #461

Merged
merged 4 commits into from Jun 11, 2014
Merged

Fix/extract long seq toscripts #461

merged 4 commits into from Jun 11, 2014

Conversation

themangoemoji
Copy link
Contributor

No description provided.

@themangoemoji
Copy link
Contributor Author

test this please

@mr-c mr-c added this to the v1.0.2 milestone Jun 10, 2014
Write out lines of FASTQ and FASTA files that exceed an argument-specified
length.

% python scripts/extract-long-sequences.py [ -l ] [ -o ] <input_file_name(s)>
Copy link
Contributor

Choose a reason for hiding this comment

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

since -l is required it shouldn't be in brackets. The easiest thing to do is to copy the relevant lines from running your script with --help here.

@mr-c
Copy link
Contributor

mr-c commented Jun 10, 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 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 spellcheck run on the source code and documentation after changes
    were made?

@mr-c
Copy link
Contributor

mr-c commented Jun 10, 2014

  • ChangeLog and I'll merge this.

& test_scripts.py & fastq-to-fasta.py & extract-long-sequences help
text.
mr-c added a commit that referenced this pull request Jun 11, 2014
@mr-c mr-c merged commit feac1bf into master Jun 11, 2014
@mr-c
Copy link
Contributor

mr-c commented Jun 11, 2014

Thanks!

@mr-c mr-c deleted the fix/extract_long_seq_toscripts branch June 11, 2014 01:35
@ctb
Copy link
Member

ctb commented Jun 11, 2014

Well, but wait. Do we have any other scripts that require a command line option? This is un-UNIX-like (as the term "option" suggests, command-line options should be optional). I would prefer either a default value of 200 (+1) or a required positional argument (as before) over a command-line option (-0 in general, -1 here since AFAIK no other scripts in khmer follow this strategy).

This needs to be addressed before 1.0.2 release, n.b.

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