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.

Sign up for GitHub

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

Jump to bottom

biom convert finalization #671

Merged
merged 5 commits into from
Oct 20, 2015
Merged

biom convert finalization #671

merged 5 commits into from
Oct 20, 2015

Conversation

jairideout
Copy link
Member

Contains commits in #670.

@jairideout jairideout added this to the 2.1.5: py2/3 compatibility milestone Oct 20, 2015
@jairideout jairideout mentioned this pull request Oct 20, 2015
Closed
@wasade
Copy link
Member

wasade commented Oct 20, 2015

@jairideout, same comment though, possible to pulldown #661?

@jairideout
Copy link
Member Author

Can we get this merged first? I don't see the benefit of pulling in #661 at this point. I need this merged to tackle #283 and #656.

@wasade
Copy link
Member

wasade commented Oct 20, 2015

The py3 changes either happen in this PR or that one... Guess it's a race
to see which gets merged first :)
On Oct 20, 2015 12:44 PM, "Jai Ram Rideout" [email protected]
wrote:

Can we get this merged first? I don't see the benefit of pulling in #661
#661 at this point. I need
this merged to tackle #283
#283 and #656
#656.

—
Reply to this email directly or view it on GitHub
#671 (comment).

@jairideout
Copy link
Member Author

I'm currently blocked and this PR is smaller, sooooooo my vote goes on this one

@wasade
Copy link
Member

wasade commented Oct 20, 2015

well... its not the size of the pr that matters. but, this one is larger by LOC...

wasade
wasade reviewed Oct 20, 2015
View reviewed changes
tests/test_cli/test_table_converter.py
self.cmd = convert
self.output_filepath = tempfile.NamedTemporaryFile().name

with tempfile.NamedTemporaryFile() as fh:
Copy link
Member

Choose a reason for hiding this comment

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

can you add mode='w'? py3 vomits otherwise

Copy link
Member

Choose a reason for hiding this comment

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

...same for other patterns like this below

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@wasade
Copy link
Member

wasade commented Oct 20, 2015

@jairideout, I marked what I think will be the primary py3 gotchas. They should be quick to do

@jairideout
Copy link
Member Author

@wasade thanks for reviewing, let me know of any other changes

@wasade
Copy link
Member

wasade commented Oct 20, 2015

looks good, 👍 merge on tests passing

wasade added a commit that referenced this pull request Oct 20, 2015
@wasade
Merge pull request #671 from jairideout/convert-finalization
00d0bbd 
biom convert finalization
@wasade wasade merged commit 00d0bbd into biocore:master Oct 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.5: py2/3 compatibility
Development

Successfully merging this pull request may close these issues.
3 participants
@jairideout @wasade @gregcaporaso