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

Mingle python 2 and 3 with ifdef #19

Merged
merged 8 commits into from
May 28, 2018
Merged

Mingle python 2 and 3 with ifdef #19

merged 8 commits into from
May 28, 2018

Conversation

dnadeau4
Copy link
Contributor

@dnadeau4 dnadeau4 commented May 7, 2018

No description provided.

@dnadeau4 dnadeau4 changed the title Mkngle python 2 and 3 with ifdef Mingle python 2 and 3 with ifdef May 7, 2018
@dnadeau4 dnadeau4 requested a review from doutriaux1 May 8, 2018 00:15
@dnadeau4 dnadeau4 mentioned this pull request May 8, 2018
@doutriaux1
Copy link
Contributor

@dnadeau4 this looks great. Thanks! In the test . I think we need to add some tests for comp<rel rel<comp etc...

Copy link
Contributor

@doutriaux1 doutriaux1 left a comment

Choose a reason for hiding this comment

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

@dnadeau4 I think we need to add some rl vs comp and comp vs rel tests and some == tests, just to be safe.

@dnadeau4
Copy link
Contributor Author

@doutriaux1 please merge.

@doutriaux1
Copy link
Contributor

@dnadeau4 please make sure we had tests for comptime vs reltime etc... as requested in my review. It's important to make sure this works.

@dnadeau4
Copy link
Contributor Author

Can you make another issue for the tests? This bug is solved.
I did add a test between comptime vs reltime using == as you wanted.

@doutriaux1
Copy link
Contributor

I'll add the tests myself when I get to this... Hopefully tomorrow... I would rather the branch to be complete before merging in.

@dnadeau4
Copy link
Contributor Author

@doutriaux1 can you merge?

@doutriaux1
Copy link
Contributor

I still need to add the test. Can this wait just another day?

@doutriaux1
Copy link
Contributor

@dnadeau4 good thing I checked and added tests. My new tests fail. Back to to drawing board 😢

@dnadeau4 dnadeau4 closed this May 28, 2018
@dnadeau4 dnadeau4 reopened this May 28, 2018
@dnadeau4 dnadeau4 merged commit 5466869 into master May 28, 2018
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