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

joda-convert and joda-time #24

Merged
merged 2 commits into from
Sep 28, 2015
Merged

Conversation

advayDev1
Copy link
Contributor

They have known issues, but we add them anyway to track when they get fixed.

Adds a proper design for multi-library projects and for libraries that
depend on other libraries we also test.

@advayDev1 advayDev1 self-assigned this Sep 26, 2015
@advayDev1 advayDev1 force-pushed the lib-joda-time branch 4 times, most recently from a0bce98 to 0ab892e Compare September 26, 2015 22:07
@advayDev1 advayDev1 changed the title joda-time joda-convert and joda-time Sep 26, 2015
@advayDev1
Copy link
Contributor Author

@brunobowden : ready to review, but you need to do all the other PRs #23 #22 #21 first.

This is what it looks like when we know some of our libraries don't translate:
https://travis-ci.org/j2objc-contrib/j2objc-common-libs-e2e-test/builds/82351797

I think it is good to have them anyway, as it documents in the logs exactly what needs to be fixed to get a working system.

#14

@advayDev1 advayDev1 assigned brunobowden and unassigned advayDev1 Sep 26, 2015
@advayDev1
Copy link
Contributor Author

#25

@brunobowden
Copy link
Contributor

The output for mixed failures and successes on builds is great. I agree that they should be separate builds to make tracking and fixes easier.

@advayDev1 advayDev1 force-pushed the lib-joda-time branch 2 times, most recently from 8835129 to 5ea60b2 Compare September 28, 2015 06:11
@advayDev1
Copy link
Contributor Author

Well that's kind of annoying of github... Once I updated this PR I don't see any of your comments on the individual commit - I'd seen them before. I see you've already LGTM-ed the next PR #26 , but please do PTAL this one. I've basically updated this PR with respect to the comments from the last one #23 . @brunobowden

@advayDev1
Copy link
Contributor Author

Fixes #14
fixes #25

@@ -7,6 +7,15 @@ os: osx
# on Traivs.
env:
- TEST_DIR=com.google.code.gson-gson
- TEST_DIR=org.joda-joda-convert
- TEST_DIR=joda-time-joda-time
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that is indeed the ugly fully qualified name for this.

like apache, joda is not consistent with its group id

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked britter about this in the other thread. Let's see if he responds with some insights.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean it is definitely correct:
https://github.com/JodaOrg/joda-time/blob/master/pom.xml#L8

just odd

They have known issues, but we add them anyway to track when they get fixed.

Adds a proper design for multi-library projects and for libraries that
depend on other libraries we also test.

Fixes j2objc-contrib#14
Fixes j2objc-contrib#25

matrix:
allow_failures:
# Blocked on https://github.com/JodaOrg/joda-convert/issues/7
Copy link
Contributor

Choose a reason for hiding this comment

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

Further expand comment:

# Should be fixed in next release: version 1.8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know that. The next version number is something they'll come up with (maybe 1.7.1, maybe 2.0, etc.), and even that we have no guarantee they will actually release. The link is sufficient - no reason to add possibly wrong info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Namely, this is a snapshot only:
https://github.com/JodaOrg/joda-convert/blob/master/pom.xml#L12

When release time comes, depending on the API changes the maintainer might need to call it 2.0, 1.7.1, etc.

@brunobowden
Copy link
Contributor

No problem, happy to go through it again. I tried doing the reviews on the individual commit... so that I could isolate review to only the part that changed. It may have been that if I did the comment on the PR, then it would've preserved the comments.

LGTM after addressing prior comments.

@advayDev1
Copy link
Contributor Author

thanks

btw: yes comments on PRs are preserved on the page (as 'outdated') even when the old commit is obliterated by git push -f; comments on commits are preserved somewhere in the backend but people have to git reflog or guess the right SHA to find the URL (assuming github doesn't GC the commit in the meantime since no ref points to it).

advayDev1 added a commit that referenced this pull request Sep 28, 2015
@advayDev1 advayDev1 merged commit 97de001 into j2objc-contrib:master Sep 28, 2015
@advayDev1 advayDev1 deleted the lib-joda-time branch September 28, 2015 20:03
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