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

Feature/android support #17

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

choutman
Copy link

Android projects are based on Java. However, the Android Gradle plugin has its own way of defining its source sets and the corresponding class paths. In an Android project it is possible to have "build flavors". This enables Android developers to build different kinds of the same Android app using the same code base. For instance a free version and a paid version of the same app.

This pull request enables Android projects to build a class diagram using this plugin. Currently, only Android projects with Kotlin are supported.

@RoRoche
Copy link
Owner

RoRoche commented Sep 10, 2020

Hi,
Thanks a lot for contributing to this project! It feeds my motivation because is a proof the concept is relevant. 😊🔋
I see that the Travis CI build failed and it reveals failing tests.
Did it run correctly locally?

@choutman
Copy link
Author

choutman commented Sep 10, 2020 via email

@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2020

Codecov Report

Merging #17 into master will increase coverage by 3.28%.
The diff coverage is 48.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #17      +/-   ##
============================================
+ Coverage     63.21%   66.50%   +3.28%     
- Complexity       31       43      +12     
============================================
  Files            18       21       +3     
  Lines           174      209      +35     
  Branches          8       13       +5     
============================================
+ Hits            110      139      +29     
  Misses           64       64              
- Partials          0        6       +6     
Impacted Files Coverage Δ Complexity Δ
.../roroche/plantuml/urls/CompileClasspathUrls.groovy 33.33% <0.00%> (+33.33%) 1.00 <0.00> (+1.00)
...oroche/plantuml/tasks/BuildClassDiagramTask.groovy 62.06% <16.66%> (-7.17%) 2.00 <0.00> (ø)
...roroche/plantuml/android/AndroidProjectType.groovy 42.85% <42.85%> (ø) 5.00 <5.00> (?)
...roroche/plantuml/urls/MainOutputUrlsFactory.groovy 55.55% <55.55%> (ø) 1.00 <1.00> (?)
...e/plantuml/urls/CompileClasspathUrlsFactory.groovy 62.50% <62.50%> (ø) 2.00 <2.00> (?)
...github/roroche/plantuml/urls/MainOutputUrls.groovy 22.22% <0.00%> (+22.22%) 1.00% <0.00%> (+1.00%)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 918e50c...efcf738. Read the comment docs.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@RoRoche
Copy link
Owner

RoRoche commented Sep 26, 2020

Oh nice you made the build passes. Well done!
I'll perform the review within a few days.
FYI I strongly follow Elegant Objects principles: https://www.elegantobjects.org/
Some rules I can't transgress are: no null, no static, no public method without a declaration in an interface. 😉
Here are some hints about feedbacks I'm gonna make.
Happy refactor 😄


}

static boolean isAndroidProject(Project project) {
Copy link
Owner

Choose a reason for hiding this comment

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

I target to don't have any static method in the codebase. Here is why: https://www.yegor256.com/2017/02/07/private-method-is-new-class.html

But looking at the source code you suggest, I guess that we could try something:

interface SuppertedProject {
  fun sourceUrls(): Urls

  abstract class Wrap(delegate: SuppertedProject) : SuppertedProject by delegate
}

class DefaultProject: SuppertedProject  {
  // do classical stuff
}

class AndroidLibrary: SuppertedProject {
  // do specific Android library stuff
}

class AndroidApplication: SuppertedProject {
  // do specific Android library stuff
}

// a impl that can determine the proper project impl to use or throw
class SmartProject(project: Project) : SupportedProject.Wrap(
  delegate = when(project) {
    project.pluginManager.hasPlugin("com.android.application") -> AndroidApplication()
    project.pluginManager.hasPlugin("com.android.library") -> AndroidLibrary()
    else -> DefaultProject()
  }
)

/**
* Factory for creating the correct classpath based on the type of a project
*/
class CompileClasspathUrlsFactory {
Copy link
Owner

Choose a reason for hiding this comment

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

Pretty much the same comment, for these reasons:

So you can move the logic to the SupportedProject suggested above.

* Factory for creating the correct classpath based on the type of a project
*/
class CompileClasspathUrlsFactory {
static Urls.Wrap createCompileClasspathUrls(Project project) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the high-level type, which is Urls in this case. The motivation is to always work by contract: https://www.yegor256.com/2014/11/20/seven-virtues-of-good-object.html#2-he-works-by-contracts

debugVariant = project.android.applicationVariants.find {
it.name.contains("debug") || it.name.contains("Debug")
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

so this logic will also go to the dedicated Android's SupportedProject.

* Factory for creating the correct output URLs based on the type of a project
*/
class MainOutputUrlsFactory {
static Urls.Wrap createMainOutputUrls(Project project) {
Copy link
Owner

Choose a reason for hiding this comment

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

So this logic can go to a KotlinProject: SupportedProject and it will be composed such as:

class KotlinProject(private val project: Project): SupportedProject {
  fun sourceUrls(): Urls {
    def debugTask = project.tasks.find {
      it.name.contains("compile") && it.name.contains("DebugKotlin")
    }
    return new MainOutputUrls(
      new FilesUrls(
        [debugTask.destinationDir].toSet()
      )
    )
  }
}

class AndroidApplication(project: Project): SupportedProject.Wrap(
  delegate = KotlinProject(project)
) {
  fun sourceUrls(): Urls {
    val ktSrcUrls = super.sourceUrls()
    // return the specific list + ktSrcUrls 
  }
}

)
)
} else {
return new MainOutputUrls(project)
Copy link
Owner

Choose a reason for hiding this comment

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

this logic goes to the default DefaultProject implementation.

project.android {
compileSdkVersion 21
}
project.evaluate()
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can reproduce the mechanism found here: https://github.com/RoRoche/plantuml-gradle-plugin/blob/master/src/test/groovy/com/github/roroche/plantuml/BuiltProject.groovy

I suggest a new interface such as:

interface TestableProject {
  fun toProject(): Project
}

and then you can put the logic to prepare a project such as:

class TestableAndroidApplication: TestableProject {
  fun toProject(): Project {
    ProjectBuilder.builder().build()
    project.apply plugin: "com.android.application"
    project.android {
      compileSdkVersion 21
    }
    project.evaluate()
    return project
  }
}

This will remove duplicate code in test.


assert AndroidProjectType.isAndroidProject(project)
assert AndroidProjectType.isAndroidApplication(project)
assert !AndroidProjectType.isAndroidLibrary(project)
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

But maybe these tests are not worth considering now and we should focus the effort on the next ones (those about Urls generation)

@choutman
Copy link
Author

Thanks for the review. As you may have noticed I did not have the time yet to make any changes. I'll try to get around it soon.

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.

3 participants