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

Add convenience methods for excluding/including individual classes #54

Merged
merged 1 commit into from
May 13, 2015

Conversation

advayDev1
Copy link
Contributor

(Please merge PR #53 first)

@advayDev1 advayDev1 force-pushed the convenience branch 2 times, most recently from edcaa11 to e4f74cb Compare May 11, 2015 00:14
@brunobowden brunobowden changed the title Add convenience methods for excluding/including individual classes. Add convenience methods for excluding/including individual classes May 13, 2015
@brunobowden
Copy link
Contributor

@advayDev1 - how do you plan to use this? I'd prefer to see it alongside code that uses it and an example. I'm also wondering if we should keep the config as a clean class and have these helper methods elsewhere... though I see the convenience of having them in the same class.

@advayDev1
Copy link
Contributor Author

they are used just like any other config method, like:

j2objcConfig {
j2objcHome 'blah'
translateExcludeClass 'advayDev1.perf.MemoryManagement'
}

(that's an actual example, I have some classes in my project that depend on memory management functionality of java.lang.System that j2objc intentionally excludes from its JRE emul)

usually gradle classes have DSL-type methods built into the classes, part of the groovy 'magic' coding style I guess. happy to put them in a separate class if you feel strongly.

@brunobowden
Copy link
Contributor

This has been in the back of my head for quite a while. The regex is powerful but will be confusing for a lot of people. I think it's best to change the config to an existing syntax that people are familiar with. The best I can think of is Gradle's CopySpec: http://gradle.org/docs/current/javadoc/org/gradle/api/file/CopySpec.html

This is a bit more involved to implement but the thought is that it could work in a very similar way. I think it's also more natural to use file paths rather than "." separated classes, as "/" is easier to generated and copy from the command line.

What are your thoughts?

j2objcConfig {
    j2objcHome 'blah'

    // Only translate the base and perf directories
    // NOTE: this can be repeated, if no entries then assume everything
    translateInclude 'base/*'
    translateInclude 'perf/*'

    // Excludes a single file and associated test:
    // a) src/main/java/perf/MemoryManagement.java
    // b) src/test/java/perf/MemoryManagementTest.java
    translateExclude 'perf/MemoryManagement*'

    // MemoryManagement is already excluded
    testExclude 'perf/SomethingElseTest.java'
}

@advayDev1
Copy link
Contributor Author

Yeah I like that idea. It means paths are automatically rooted at the source set root, which is consistent with gradle plugin conventions. Looks like we can use the more general:
http://gradle.org/docs/current/javadoc/org/gradle/api/tasks/util/PatternSet.html
which provides a general Glob interface like CopySpec but standalone.

Might be good to refactor the translation task to delegate to or extend this too (which handles PatternFilterable intrinsically):
http://gradle.org/docs/current/javadoc/org/gradle/api/tasks/SourceTask.html

@brunobowden
Copy link
Contributor

I'm merging for now but this ideally should be only temporary until you can switch to a glob / patternset model. See note about writing up that issue and adding TODO pointing to the same issue.

brunobowden added a commit that referenced this pull request May 13, 2015
Add convenience methods for excluding/including individual classes
@brunobowden brunobowden merged commit f848852 into j2objc-contrib:master May 13, 2015
@advayDev1 advayDev1 deleted the convenience branch May 13, 2015 07:04
@advayDev1 advayDev1 restored the convenience branch May 13, 2015 07:07
@advayDev1 advayDev1 deleted the convenience branch May 13, 2015 07:34
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