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

Config to only add j2objc_shared method to Podfile #565

Closed
wants to merge 7 commits into from
Closed

Config to only add j2objc_shared method to Podfile #565

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
86004d5
start working on https://github.com/j2objc-contrib/j2objc-gradle/issu…
saruye Nov 8, 2015
8afd2b6
work on PR
saruye Nov 16, 2015
4f8d7ed
work on PR
saruye Nov 16, 2015
1aa92c7
work on PR
saruye Nov 22, 2015
605b623
Merge branch 'master' into only-update-podfile
saruye Nov 22, 2015
555bfad
work on PR
saruye Nov 22, 2015
36e8d3c
Merge branch 'only-update-podfile' of https://github.com/saruye/j2obj…
saruye Nov 22, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/main/groovy/com/github/j2objccontrib/j2objcgradle/J2objcConfig.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,8 @@ class J2objcConfig {
*/
String xcodeProjectDir = null

boolean onlyAddJ2ObjcToPodfile = false
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Rename so that it's more clearly associated with the xcodeTargetsIos config:
xcodeTargetsManualConfig
  1. Add the javadoc to it indicating what it does:
/**
 * Allows manual config of the Podfile (default is false).
 *
 * When set to true, this allows manual configuring of the Podfile targets.
 * This is necessary when your Podfile is too complex to be automatically
 * updated. When used, you must also set xcodeTargets{Ios|Osx|Watchos)
 * to empty.
 */
  1. move this after the xcodeTargetsWatchos setting, so it's the last one listed.


/**
* iOS app and test Xcode targets to link to the generated libraries.
*
Expand Down
51 changes: 29 additions & 22 deletions src/main/groovy/com/github/j2objccontrib/j2objcgradle/tasks/XcodeTask.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ class XcodeTask extends DefaultTask {
@Input @Optional
String getXcodeProjectDir() { return J2objcConfig.from(project).xcodeProjectDir }

@Input
boolean isOnlyAddJ2ObjcToPodfile() { return J2objcConfig.from(project).onlyAddJ2ObjcToPodfile }
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow the existing convention and use a "get" prefix on the existing name, with first letter capitalized:

getXcodeTargetsManualConfig


boolean isTaskActive() { return getXcodeProjectDir() != null }

@Input
Expand Down Expand Up @@ -200,7 +203,7 @@ class XcodeTask extends DefaultTask {
getXcodeTargetsIos(), getXcodeTargetsOsx(), getXcodeTargetsWatchos(),
getMinVersionIos(), getMinVersionOsx(), getMinVersionWatchos())

writeUpdatedPodfileIfNeeded(podspecDetailsList, xcodeTargetDetails, podfile)
writeUpdatedPodfileIfNeeded(podspecDetailsList, xcodeTargetDetails,!isOnlyAddJ2ObjcToPodfile(), podfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

By using a similar variable name (and meaning) throughout the code, you should avoid things like negation. When you make a change like this, a developer has to think "it's X over here" then "it's not-X over here". That's a significant addition in mental complexity for someone else reading the code that you should avoid.


// install the pod
ByteArrayOutputStream stdout = new ByteArrayOutputStream()
Expand Down Expand Up @@ -374,14 +377,14 @@ class XcodeTask extends DefaultTask {
@VisibleForTesting
static void writeUpdatedPodfileIfNeeded(
List<PodspecDetails> podspecDetailsList,
XcodeTargetDetails xcodeTargetDetails,
XcodeTargetDetails xcodeTargetDetails, boolean updateTargets,
Copy link
Contributor

Choose a reason for hiding this comment

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

Follow existing conventions and put new parameter on separate line.

Also, rename to xcodeTargetsManualConfig.... I strongly believe in consistent naming so that it's easier for a developer to understand code as they're reading it.

File podfile) {

List<String> oldPodfileLines = podfile.readLines()
List<String> newPodfileLines = new ArrayList<String>(oldPodfileLines)

newPodfileLines = updatePodfile(
newPodfileLines, podspecDetailsList, xcodeTargetDetails, podfile)
newPodfileLines, podspecDetailsList, xcodeTargetDetails,updateTargets, podfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need space after comma.


// Write file only if it's changed
if (!oldPodfileLines.equals(newPodfileLines)) {
Expand All @@ -393,34 +396,38 @@ class XcodeTask extends DefaultTask {
static List<String> updatePodfile(
List<String> podfileLines,
List<PodspecDetails> podspecDetailsList,
XcodeTargetDetails xcodeTargetDetails,
XcodeTargetDetails xcodeTargetDetails,boolean updateTargets,
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameter on new line.

Rename to xcodeTargetsManualConfig.

File podfile) {

List<String> podfileTargets = extractXcodeTargets(podfileLines)
verifyTargets(xcodeTargetDetails.xcodeTargetsIos, podfileTargets, 'xcodeTargetsIos')
verifyTargets(xcodeTargetDetails.xcodeTargetsOsx, podfileTargets, 'xcodeTargetsOsx')
verifyTargets(xcodeTargetDetails.xcodeTargetsWatchos, podfileTargets, 'xcodeTargetsWatchos')
if(updateTargets){
Copy link
Contributor

Choose a reason for hiding this comment

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

Need space between if and (.

List<String> podfileTargets = extractXcodeTargets(podfileLines)
verifyTargets(xcodeTargetDetails.xcodeTargetsIos, podfileTargets, 'xcodeTargetsIos')
verifyTargets(xcodeTargetDetails.xcodeTargetsOsx, podfileTargets, 'xcodeTargetsOsx')
verifyTargets(xcodeTargetDetails.xcodeTargetsWatchos, podfileTargets, 'xcodeTargetsWatchos')

if (xcodeTargetDetails.xcodeTargetsIos.isEmpty() &&
xcodeTargetDetails.xcodeTargetsOsx.isEmpty() &&
xcodeTargetDetails.xcodeTargetsWatchos.isEmpty()) {
// Give example for configuring iOS as that's the common case
throw new InvalidUserDataException(
"You must configure the xcode targets for the J2ObjC Gradle Plugin.\n" +
"It must be a subset of the valid targets: '${podfileTargets.join("', '")}'\n" +
"\n" +
"j2objcConfig {\n" +
" xcodeTargetsIos 'IOS-APP', 'IOS-APPTests' // example\n" +
"}\n" +
"\n" +
"Can be optionally configured for xcodeTargetsOsx and xcodeTargetsWatchos\n")
if (xcodeTargetDetails.xcodeTargetsIos.isEmpty() &&
xcodeTargetDetails.xcodeTargetsOsx.isEmpty() &&
xcodeTargetDetails.xcodeTargetsWatchos.isEmpty()) {
// Give example for configuring iOS as that's the common case
throw new InvalidUserDataException(
"You must configure the xcode targets for the J2ObjC Gradle Plugin.\n" +
"It must be a subset of the valid targets: '${podfileTargets.join("', '")}'\n" +
"\n" +
"j2objcConfig {\n" +
" xcodeTargetsIos 'IOS-APP', 'IOS-APPTests' // example\n" +
"}\n" +
"\n" +
"Can be optionally configured for xcodeTargetsOsx and xcodeTargetsWatchos\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Add additional line:

This check can be disabled with "xcodeTargetsManualConfig = true"

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rewrite logic so that it forces xcode targets to be empty when doing manual config. It should be either or when configuring this.

boolean xcodeTargetsAllEmpty =
        xcodeTargetDetails.xcodeTargetsIos.isEmpty() &&
        xcodeTargetDetails.xcodeTargetsOsx.isEmpty() &&
        xcodeTargetDetails.xcodeTargetsWatchos.isEmpty()

if (xcodeTargetsManualConfig) {
    if (!xcodeTargetsAllEmpty) {
        throw new InvalidUserDataException(
                "Xcode targets must all be blank when using xcodeTargetsManualConfig.\n" +
                "Please update j2objcConfig by removing xcodeTargetsIos, xcodeTargetsOsx & xcodeTargetsWatchos")
    }
} else {
    // xcodeTargetsManualConfig = false  (default)
    List<String> podfileTargets = extractXcodeTargets(podfileLines)
    verifyTargets(...
    if (xcodeTargetsAllEmpty) {
        existing exception
    }
}

}

// update pod methods
List<String> newPodfileLines = updatePodMethods(podfileLines, podspecDetailsList, podfile)

// update pod targets
newPodfileLines = updatePodfileTargets(newPodfileLines, podspecDetailsList, xcodeTargetDetails)
if(updateTargets){
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine this section with the previous if (updateTargets) section. For mental complexity, that conditional should all be together. Then likely put line 425, the "updatePodMethods" call before that section again.

This is how it is currently:

if (a) {
   foo1()
}

bar()

if (a) {
   foo2()
}

See how much simpler it is to understand this instead:

bar()

if (a) {
   foo1()
   foo2()
}

newPodfileLines = updatePodfileTargets(newPodfileLines, podspecDetailsList, xcodeTargetDetails)
}

return newPodfileLines
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For the verityTargets method below (I have to comment on this line as I can't comment in the review on a line that hasn't been modified). Within that method, add another line to explain how to handle a complex Podfile. My supposition is that in that scenario, this is where it will fail, so it's the most useful spot to add an explanation.

private static verifyTargets(List<String> xcodeTargets, List<String> podfileTargets, xcodeTargetsName) {
    xcodeTargets.each { String xcodeTarget ->
        if (! podfileTargets.contains(xcodeTarget)) {
            throw new InvalidUserDataException(
                    "Invalid j2objcConfig { $xcodeTargetsName '$xcodeTarget' }\n" +
                    "Must be one of the valid targets: '${podfileTargets.join("', '")}'\n" +
                    "NOTE: if your Podfile is too complex, you may need to use xcodeTargetsManualConfig")
        }
    }
}

Expand Down
67 changes: 59 additions & 8 deletions src/test/groovy/com/github/j2objccontrib/j2objcgradle/tasks/XcodeTaskTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,7 +430,7 @@ class XcodeTaskTest {
new File(podspecBuildDir + '/j2objc-PROJ-debug.podspec'),
new File(podspecBuildDir + '/j2objc-PROJ-release.podspec')))
XcodeTask.writeUpdatedPodfileIfNeeded(
podspecDetailsList, xcodeTargetDetailsIosAppOnly, podfile)
podspecDetailsList, xcodeTargetDetailsIosAppOnly, true, podfile)

// Verify modified Podfile
List<String> expectedLines = [
Expand All @@ -451,7 +451,7 @@ class XcodeTaskTest {
// Verify unmodified on second call
// TODO: verify that file wasn't written a second time
XcodeTask.writeUpdatedPodfileIfNeeded(
podspecDetailsList, xcodeTargetDetailsIosAppOnly, podfile)
podspecDetailsList, xcodeTargetDetailsIosAppOnly, true, podfile)
readPodfileLines = podfile.readLines()
assert expectedLines == readPodfileLines
}
Expand All @@ -465,7 +465,7 @@ class XcodeTaskTest {
List<String> newPodfileLines = XcodeTask.updatePodfile(
podfileLines,
podspecDetailsProj,
xcodeTargetDetailsIosAppOnly,
xcodeTargetDetailsIosAppOnly, true,
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's already a convention of putting each parameter on a new line, then do it. It's confusing to mix styles, you should adopt the same local style that already exists.

new File('/SRC/ios/Podfile'))

List<String> expectedPodfileLines = [
Expand All @@ -485,7 +485,39 @@ class XcodeTaskTest {
newPodfileLines = XcodeTask.updatePodfile(
newPodfileLines,
podspecDetailsProj,
xcodeTargetDetailsIosAppOnly,
xcodeTargetDetailsIosAppOnly, true,
new File('/SRC/ios/Podfile'))
assert expectedPodfileLines == newPodfileLines
}

@Test
void testUpdatePodfile_onlyAddMethod() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename to: testUpdatePodfile_xcodeTargetsManualConfig

List<String> podfileLines = [
"target 'IOS-APP' do",
"end"]
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 this is better demonstrated with a more complex Podfile example. Where there loops or other strange ruby logic that is impossible to update normally. That's the important example to have here.


List<String> newPodfileLines = XcodeTask.updatePodfile(
podfileLines,
podspecDetailsProj,
xcodeTargetDetailsIosAppOnly, false,
new File('/SRC/ios/Podfile'))

List<String> expectedPodfileLines = [
"# J2ObjC Gradle Plugin - DO NOT MODIFY from here to the first target",
"def j2objc_PROJ",
" pod 'j2objc-PROJ-debug', :configuration => ['Debug'], :path => '../PROJ/BUILD'",
" pod 'j2objc-PROJ-release', :configuration => ['Release'], :path => '../PROJ/BUILD'",
"end",
"",
"target 'IOS-APP' do",
"end"]
assert expectedPodfileLines == newPodfileLines

// Second time around is a no-op
newPodfileLines = XcodeTask.updatePodfile(
newPodfileLines,
podspecDetailsProj,
xcodeTargetDetailsIosAppOnly, false,
Copy link
Contributor

Choose a reason for hiding this comment

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

New line for new parameter

new File('/SRC/ios/Podfile'))
assert expectedPodfileLines == newPodfileLines
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add another test where it guides the user on how to handle a complex, non-standard Podfile:

testUpdatePodfile_complexPodfileNeedsXcodeTargetsManualConfig() {
    List<String> podfileLines = [
        // Construct target name by concatenation, in a way that automated detection fails
        "target ('IOS-' + 'APP') do",
        "end"]

    expectedException.expect(InvalidUserDataException.class)
    expectedException.expectMessage('NOTE: if your Podfile is too complex, you may need to use xcodeTargetsManualConfig')

    // Second time around is a no-op
    newPodfileLines = XcodeTask.updatePodfile(
        newPodfileLines,
        podspecDetailsProj,
        xcodeTargetDetailsIosAppOnly,
        false,
        new File('/SRC/ios/Podfile'))
}

Expand All @@ -509,7 +541,7 @@ class XcodeTaskTest {
List<String> newPodfileLines = XcodeTask.updatePodfile(
podfileLines,
podspecDetailsProj,
xcodeTargetDetails,
xcodeTargetDetails, true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adopt local style of new line for each parameter

new File('/SRC/ios/Podfile'))

List<String> expectedPodfileLines = [
Expand Down Expand Up @@ -539,7 +571,7 @@ class XcodeTaskTest {
newPodfileLines = XcodeTask.updatePodfile(
newPodfileLines,
podspecDetailsProj,
xcodeTargetDetails,
xcodeTargetDetails, true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adopt local style of new line for each parameter

new File('/SRC/ios/Podfile'))
assert expectedPodfileLines == newPodfileLines
}
Expand All @@ -562,7 +594,7 @@ class XcodeTaskTest {
[],
new XcodeTask.XcodeTargetDetails(
[], [], [],
'6.0.0', '10.6.0', '1.0.0'),
'6.0.0', '10.6.0', '1.0.0'), true,
null)
}

Expand All @@ -583,10 +615,29 @@ class XcodeTaskTest {
[],
new XcodeTask.XcodeTargetDetails(
['TARGET-DOES-NOT-EXIST'], [], [],
'6.0.0', '10.6.0', '1.0.0'),
'6.0.0', '10.6.0', '1.0.0'), true,
null)
}

@Test
void testUpdatePodfileNoTargets_onlyAddMethod() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The first part of the test name is the method under test, so the "NoTargets" part should not be there.

This doesn't seem to add much value compared to the existing testUpdatePodfile_xcodeTargetsManualConfig test. I'm not sure if it's worth having it.

A better test would be a more complex but still valid Podfile. Perhaps based on your own which I presume meant that you came across this problem. In any case, if you add that, then it should likely be next to the testUpdatePodfile_xcodeTargetsManualConfig test, perhaps named testUpdatePodfile_xcodeTargetsManualConfigComplexPodfile.

List<String> podfileLines = []

List<String> newPodfileLines = XcodeTask.updatePodfile(
podfileLines, podspecDetailsProj,
xcodeTargetDetailsIosAppOnly, false,
new File('/SRC/ios/Podfile'))

List<String> expectedLines = [
"# J2ObjC Gradle Plugin - DO NOT MODIFY from here to the first target",
"def j2objc_PROJ",
" pod 'j2objc-PROJ-debug', :configuration => ['Debug'], :path => '../PROJ/BUILD'",
" pod 'j2objc-PROJ-release', :configuration => ['Release'], :path => '../PROJ/BUILD'",
"end"]

assert expectedLines == newPodfileLines
}

@Test
void testUpdatePodMethods_basic() {
List<String> podfileLines = [
Expand Down