-
Notifications
You must be signed in to change notification settings - Fork 295
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
Promote error correction scripts from sandbox #1743
base: master
Are you sure you want to change the base?
Conversation
thanks, Will, this is excellent!
If you separate the move to scripts/ from the edits in sandbox/, this will
likely get merged much faster.
|
Awesome! In addition to @ctb's comment, see http://khmer.readthedocs.io/en/latest/dev/scripts-and-sandbox.html#upgrading-a-script-from-sandbox-to-scripts. |
Codecov Report
@@ Coverage Diff @@
## master #1743 +/- ##
=========================================
- Coverage 0.05% 0.05% -0.01%
=========================================
Files 90 92 +2
Lines 11336 11572 +236
Branches 2992 3032 +40
=========================================
Hits 6 6
- Misses 11330 11566 +236
Continue to review full report at Codecov.
|
FailToUnderstandError: Do you want another issue, to separate move vs. patch, or do you want me to clean up the commits here? |
Lol
It doesn't matter much whether you adjust this PR or make a new one: whichever is least inconvenient to you. But it would help the review process to separate 1) the changes you've been making to the scripts from 2) the changes that will be required to upgrade a script from Let me know if that computes. ;-) |
The automatic build/testing doesn't find scripts that are in sandbox, so the tests I added failed; i thought I had to move the scripts (or the testing would fail horribly) Is force pushing to a PR branch acceptable after rebasing, or is this impolite to git and travis? Any idea why codecov doesn't seem to be reporting accurately? |
30632b7
to
6c333f2
Compare
force pushing is fine. @betatim any thoughts re codecov? |
I do it all the time coughcough when I rebase my PR/amend commits. The only downside might be that inline comments sometimes get collapsed but other than that ... no one ever notices it seems :p |
This is an attempt to bring two (of only three) CLI scripts that use ReadAligner up to date with the khmer codebase,
error_correct_pass2
andcorrect-reads
. Tests were added, and the scripts were moved fromsandbox
toscripts
.make test
Did it pass the tests?make clean diff-cover
If it introduces new functionality inscripts/
is it tested?make format diff_pylint_report cppcheck doc pydocstyle
Is it wellformatted?
additions are allowed without a major version increment. Changing file
formats also requires a major version number increment.
documented in
CHANGELOG.md
? See keepachangelogfor more details.
changes were made?
tested for streaming IO?)
Checklist for moving scripts from sandbox to scripts:
make diff-cover
)make diff_pylint
is cleanget_parser()
and added to doc/user/scripts.txtkhmer_args.info(...)
)