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: transformers SingleLanePerSample... to Casava #245

Merged
merged 5 commits into from May 29, 2020
Merged

IMP: transformers SingleLanePerSample... to Casava #245

merged 5 commits into from May 29, 2020

Conversation

thermokarst
Copy link
Contributor

Fixes #209

Related to qiime2/q2-quality-control#52


Missing unit tests

@thermokarst
Copy link
Contributor Author

Relevant discussion here: qiime2/q2-quality-control#52

I'm not sure yet about the transformer here

https://github.com/qiime2/q2-types/pull/245/files#diff-3fd5d1462a2b3d80c0d3c98949f10949R94-R102

The problem is that it ends up producing a useless basepath, which ultimately needs to be rewritten in the format property. Perhaps it would be better to eliminate that transformer altogether and stick it all in the property, I was just trying to recycle as much of the transformers functionality as I possibly could. Open to ideas.

@thermokarst
Copy link
Contributor Author

I'm not sure yet about the transformer here

The more I think about this, the more I want to refactor as internal utils used by the fmt property && the transformers, and dropping this transformer.

@thermokarst thermokarst assigned thermokarst and unassigned nbokulich May 27, 2020
@thermokarst
Copy link
Contributor Author

Okay, the diff is pretty gross because there is a bit of code churn, but the refactor looks like this: first I moved all the utils out of _transformer.py and into a new shared _util.py. The new util module is used by the transformers module && the formats module, so I had to refactor the utils so that they injected their format dependencies at runtime (I am open to other suggestions, just let me know). Then I created some new dynamic utils via functool, to cut down on the boilerplate. Lastly, I refactored the dynamic manifest property on the format to use the shared utils.

@thermokarst thermokarst assigned nbokulich and unassigned thermokarst May 27, 2020
@thermokarst
Copy link
Contributor Author

BTW - this is still missing unit tests, but I can add those as part of my response-to-review.

@thermokarst thermokarst assigned thermokarst and unassigned nbokulich May 28, 2020
@thermokarst thermokarst assigned nbokulich and unassigned thermokarst May 28, 2020
Copy link
Member

@nbokulich nbokulich left a comment

Choose a reason for hiding this comment

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

Thanks @thermokarst ! This LGTM, I just have one inline comment.

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
2 participants