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

Changes to dependencies for cleanliness in Android #30

Merged
merged 1 commit into from Jun 16, 2013

Conversation

billmag
Copy link
Contributor

@billmag billmag commented Apr 23, 2013

Not sure if you want this, but the commons-collections dependency is kind of unruly and seemed unnecessary to me. I also changed the org.json version to be the one included with Android so that there aren't version conflicts when this is used with Android.

…son dependency to the same version included with Android. This makes including this as a library in an Android project cleaner/easier.
@carterpage
Copy link
Member

I like the idea of ditching commons-collections. Can you provide a unit test with?

We can't go backwards on the JSON library. The 2009 version is buggy as it is, and we'll probably end up cutting a support library off the latest source code to use, since @douglascrockford doesn't seem interested in releasing a new version on maven.

@@ -52,15 +52,9 @@
<dependency>
<groupId>org.json</groupId>
<artifactId>json</artifactId>
<version>20090211</version>
<version>20080701</version>
Copy link
Member

Choose a reason for hiding this comment

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

Please undo this change. We can't go to an older JSON library. More than just android projects rely on us.

@billmag
Copy link
Contributor Author

billmag commented Apr 23, 2013

Would you be OK with setting a floor for the JSON version and allowing 2008, but not mandating it? If we include the 2009 version as well, Proguard does annoying things when packaging up against the Android libraries unless you explicitly exclude it from the transitive dependency list for this library.

@carterpage
Copy link
Member

What would that be? Something like this?

<version>[20080701,)</version>

@billmag
Copy link
Contributor Author

billmag commented May 8, 2013

Something like that should probably work. I'll need to test it, which I'll
do next week if no one else does in the meantime.

On Tue, Apr 23, 2013 at 6:35 PM, Carter Page notifications@github.comwrote:

What would that be? Something like this?

[20080701,)


Reply to this email directly or view it on GitHubhttps://github.com//pull/30#issuecomment-16891218
.

Bill Magnuson* @billmag* :: Co-Founder & CTO ::
appboy.comhttp://www.appboy.com/
Appboy is Young Entrepreneur's startup of the
monthhttp://www.youngentrepreneur.com/startingup/start-ups/appboy-founder-on-turning-a-chance-encounter-into-entrepreneurship-gold-2/
!

@carterpage
Copy link
Member

I've thought about it , and I'm not going to approve the maven change. Eventually we're going to release our own support version of JSON via Maven and depend on that because the artifacts have gotten so far out of date and is causing bugs in JSONAssert. Android developers, as well as anyone using needing to compile JSONAssert with an older version of JSON will need to use maven excludes.

I'd like to merge the work for getCardinalityMap, but I need two things first:

  1. Write a set of thorough tests for the new functionality.
  2. Undo the pom.xml change.

Follow up here if you have any questions. Thanks!

carterpage added a commit that referenced this pull request Jun 16, 2013
Changes to dependencies for cleanliness in Android.

I'm going to accept the merge then follow up with a unit test and reverting the JSON version to the current one.
@carterpage carterpage merged commit 452e69d into skyscreamer:master Jun 16, 2013
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.

None yet

2 participants