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

skip trimming option #124

Merged
merged 6 commits into from Dec 8, 2016
Merged

skip trimming option #124

merged 6 commits into from Dec 8, 2016

Conversation

mortonjt
Copy link
Collaborator

@mortonjt mortonjt commented Dec 7, 2016

This adds an option to ignore trimming.

So you should be able to specify a flag to completely ignore trimming.

Here a test case is added. In addition, I have tested out the CLI on some of the test datasets.
Note that this is a little tricky to test, so any suggestions on improving this will be welcome.

But I believe that this should cover the gist of it.

@wasade
Copy link
Member

wasade commented Dec 7, 2016

thanks! there are some failures btw

cc @antgonza

@mortonjt
Copy link
Collaborator Author

mortonjt commented Dec 7, 2016

A fix for the file path is coming in - its passing on my system. But this is ready for review.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 88.98% when pulling b1c0c8c on mortonjt:version-1.7 into b377d4a on biocore:master.

Copy link
Contributor

@antgonza antgonza left a comment

Choose a reason for hiding this comment

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

One comment.

input_seqs=sequence_generator(seqs_fp), trim_len=trim_length):
out_f.write(">%s\n%s\n" % (label, seq))
if skip_trim:
for label, seq in sequence_generator(seqs_fp):
Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically duplicating the file, right? Can we do something smarter?

Copy link
Member

Choose a reason for hiding this comment

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

What about creating a simple symlink?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's cool! I didn't know about that.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 89.162% when pulling a5e4b7e on mortonjt:version-1.7 into b377d4a on biocore:master.

@mortonjt
Copy link
Collaborator Author

mortonjt commented Dec 8, 2016

Ok. I think this is ready to merge!

@antgonza antgonza merged commit fb8858b into biocore:master Dec 8, 2016
@mortonjt
Copy link
Collaborator Author

mortonjt commented Dec 8, 2016

Thanks!

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

5 participants