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

Adds testing scripts and Gson as first library. #23

Merged
merged 1 commit into from
Sep 28, 2015

Conversation

advayDev1
Copy link
Contributor

Fixes #21
Fixes #19

Do #22 first.

env:
- USING_OS=osx USING_XCODE=6.1
- TEST_DIR=gson
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the directory names should follow the package name convention, replacing : with -. In this case:

com.google.code.gson-gson

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@brunobowden
Copy link
Contributor

Please respond to the comments. Close to LGTM.

@brunobowden
Copy link
Contributor

Note that the prior LGTM was a comment on a related PR, not this specific one. Ping me again when thsi is ready for another round.

@advayDev1
Copy link
Contributor Author

@brunobowden ptal - I will flatten the 2 commits once LGTY


j2objcConfig {
// Multiple Java filenames can indeed be identical.
filenameCollisionCheck false
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be done by default... if it is really needed, I was expecting a comment like:

// Suppress existing filename collision:
// src/main/abc/File1.java
// src/main/xyz/File1.java

Are you using no-package-directories for the translation? If there is a real collision going on, then I think that prefixes.properties can help fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. prefixes.properties does not affect the filename, just the class name. Also, these libraries in our repo should never use prefixes.properties. Unless all transitive forward and backward dependencies of a library use the same prefixes.properties files, you are not guaranteed they will be able work with each other.
  2. We aren't using no-package-directories, however the plugin checks for filename collisions regardless of no-package-directories. That is a bug in the plugin, filed: filenameCollisionCheck should default to the value of no-package-directories j2objc-gradle#467 - so until that is fixed it should indeed be set to false - because files in distinct directory do not indeed collide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, package-info.java exists in multiple packages in all of these libraries.

@brunobowden
Copy link
Contributor

LGTM after addressing last 3 comments.

@advayDev1
Copy link
Contributor Author

all done - merging.

advayDev1 added a commit that referenced this pull request Sep 28, 2015
Adds testing scripts and Gson as first library.
@advayDev1 advayDev1 merged commit f1c0aef into j2objc-contrib:master Sep 28, 2015
@advayDev1 advayDev1 deleted the lib-gson branch September 28, 2015 05:42
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