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

Add toil-lib dependency (resolves BD2KGenomics/toil#922) #434

Merged
merged 4 commits into from
Sep 6, 2016

Conversation

jvivian
Copy link
Collaborator

@jvivian jvivian commented Sep 6, 2016

No description provided.

@fnothaft
Copy link
Contributor

fnothaft commented Sep 6, 2016

I like the +/- spread on this! Reviewing now.

@@ -73,6 +73,7 @@ def check_provided(distribution, min_version, max_version=None, optional=False):
author_email='[email protected]',
url="https://github.com/BD2KGenomics/toil-scripts",
install_requires=[
'toil-lib==1.0.2',
'tqdm==3.8.0', # FIXME: Remove once ADAM stops using it (superfluous import)
Copy link
Contributor

Choose a reason for hiding this comment

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

Small aside, but I think we can get rid of the tqdm requirement. I don't see it used anywhere in the codebase.

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 would be great

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just remove it in this PR?

@fnothaft
Copy link
Contributor

fnothaft commented Sep 6, 2016

Just dropped a few nits. Other than nits, LGTM!

@jvivian
Copy link
Collaborator Author

jvivian commented Sep 6, 2016

Thanks @fnothaft! I'll address these now.

@jvivian
Copy link
Collaborator Author

jvivian commented Sep 6, 2016

@fnothaft — found a blunder in toil-lib. I'll need to push 1.0.3 out before I can merge this, doing that now.

@fnothaft
Copy link
Contributor

fnothaft commented Sep 6, 2016

Blimey mate! A blunder!

@jvivian jvivian merged commit 2e47d32 into master Sep 6, 2016
@jvivian jvivian deleted the issues/toil-922-add-toil-lib branch September 6, 2016 22:14
@fnothaft
Copy link
Contributor

fnothaft commented Sep 6, 2016

@jvivian so now that this is merged, PRs that added to toil_scripts.tools or toil_scripts.lib should be split, right? E.g., #381 added to toil_scripts.tools.qc. Is the new workflow that we open a PR against toil-lib, get that merged, bump the toil-lib version, and then PR the rest here? Or, actually, should new functionality go into it's own repository? E.g., should #381 be split into a PR against toil-lib and a new repository?

@jvivian
Copy link
Collaborator Author

jvivian commented Sep 6, 2016

That's a good question. I think the former option is what I'll do in the short term until a pipeline gets broken out into its own repo. Once a pipeline has broken up with its estranged lover toil-scripts, PRs can target the new repo with changes that complement the additions to toil-lib.

PR against toil-lib > merge > bump toil-lib to new release > build release branch on jenkins to push to pypi > bump toil-lib to dev version > PR against toil-scripts | PR against pipeline-repo.

@fnothaft
Copy link
Contributor

fnothaft commented Sep 6, 2016

SGTM. I'm not planning to touch #381 for a while, so I'll hold off there.

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.

2 participants