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

IMP: Add transformer from slpssefdf to Casava #226

Closed
wants to merge 8 commits into from

Conversation

Oddant1
Copy link
Member

@Oddant1 Oddant1 commented Aug 26, 2019

Closes #209

@thermokarst thermokarst added this to In Review in 2019.10 via automation Aug 26, 2019
@ebolyen ebolyen moved this from In Review to In Development in 2019.10 Aug 29, 2019
@ebolyen ebolyen assigned Oddant1 and unassigned thermokarst Aug 29, 2019
@Oddant1
Copy link
Member Author

Oddant1 commented Sep 2, 2019

The transformer in 3f357bc now copies all non MANIFEST/metadata files to a temp directory and creates the casavadirfmt from that, I did this because I didn't think we wanted to just kill off the MANIFEST and metadata files, but I can just kill them and use the directory that was passed in to the transformer if we don't care about them anymore.

@Oddant1 Oddant1 assigned ebolyen and unassigned Oddant1 Sep 2, 2019
@Oddant1 Oddant1 moved this from In Development to In Review in 2019.10 Sep 9, 2019
@@ -351,7 +360,7 @@ def _9(fmt: PairedEndFastqManifestPhred64) \


@plugin.register_transformer
def _12(dirfmt: SingleLanePerSampleSingleEndFastqDirFmt) \
def _27(dirfmt: SingleLanePerSampleSingleEndFastqDirFmt) \
Copy link
Contributor

Choose a reason for hiding this comment

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

DBC: Why not keep this as _12, and name the new one _27? Just so you know, with transformers the names of the functions don't matter, we just took the strategy of naming them _number, but it can be anything

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, probably just because I saw _10, _11 and was bothered at the idea of the next thing being _27 but I guess the rest isn't in numerical order anyway 😆

@thermokarst
Copy link
Contributor

Hey there @ChrisKeefe, can you please review this PR when you get a chance? Please address any changes you might want made to @ebolyen & I - we are teaming up to run this one across the finish line!

@ebolyen ebolyen assigned ebolyen and unassigned ChrisKeefe Oct 10, 2019
@ebolyen ebolyen moved this from In Review to In Development in 2019.10 Oct 10, 2019
@thermokarst thermokarst removed this from In Development in 2019.10 Oct 30, 2019
@thermokarst thermokarst added this to In Review in 2020.2 Oct 31, 2019
@ebolyen
Copy link
Member

ebolyen commented Nov 8, 2019

This isn't a high priority, and there's a few issues, so we'll just close this for now, so that everyone can focus on other things.

@ebolyen ebolyen closed this Nov 8, 2019
@ebolyen ebolyen removed this from In Review in 2020.2 Nov 8, 2019
@Oddant1
Copy link
Member Author

Oddant1 commented Jan 24, 2020

@ebolyen can we reopen this PR and finish up this issue?

@Oddant1 Oddant1 changed the title IMP: Add transformer from casava to slpssefdf IMP: Add transformer from slpssefdf to Casava Jan 24, 2020
@ebolyen
Copy link
Member

ebolyen commented Jan 24, 2020

I think this is still pretty low priority, I'm tempted to move it off the backlog, but it would be nice to have in the future. Let's hold off for now.

@Oddant1 Oddant1 deleted the singlelane-to-casava branch June 25, 2020 20:41
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.

Transform SLPSSEFDF -> CasavaOneEightSingleLanePerSampleDirFmt
4 participants