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

Fix tools.forma.includer plugin behavior #128

Merged
merged 4 commits into from
Jul 8, 2023

Conversation

tonykolomeytsev
Copy link
Collaborator

@tonykolomeytsev tonykolomeytsev commented Jun 29, 2023

Fixed some problems of the Includer plugin that prevent its integration with our project:

  • Skip including nested projects.
    Current behavior: nested projects with settings.gradle(.kts) are not added, but their subprojects are added.
    New behavior: nested projects with settings.gradle(.kts) and their subprojects are completely skipped.
  • Make plugin configurable:
    • Allowed to disable arbitrary build file names with property tools.forma.includer.arbitraryBuildFileNames. For those cases when we want the plugin to include only modules with build.gradle(.kts) files.
    • Allowed to specify additional folder names to ignore with property tools.forma.includer.ignoredFolders.
  • Fail project configuration if a module with more than one build file is encountered (with a very clear error description).
  • Extend IncluderPluginFunctionalTest to cover all plugin use cases.

- Skip including nested projects
- Consider only dirs with `build.gradle(.kts)` files as subprojects
- Extend `IncluderPluginFunctionalTest` to cover problematic use cases
- Allowed to disable arbitrary build file names
- Allowed to specify additional folder names to ignore
- Do not allow to configure the project if a module with more than one build file is encountered
- Covered all use cases of the plugin with tests
- Added documentation to the README
@stepango
Copy link
Collaborator

stepango commented Jul 7, 2023

Looks great! One concern from my side, using gradle.properties file for configuration.
I suggest we don't use gradle.properties for configuration of forma related plugins:

  • it reduces visibility, you have to use 2 files to configure the plugin
  • It created confusion because for every functionality we'll need to decide if we want to configure true properties or extensions
  • or even worse scenario if we have to support two options which prone to error and confusion about ordering priority
includer {
  excludeFolders("scripts", "tmp")
  arbitraryBuildScriptNames = true
}

- Create IncluderExtension and Dsl function `includer` for it
- Add docs for IncluderExtension properties
- Disable arbitraryBuildScriptNames by default
- Covered all use cases of the plugin with tests
- Update documentation in the README
@tonykolomeytsev
Copy link
Collaborator Author

@stepango, good point 👍

I fixed it. Now we can use type-safe includer { ... } extension instead of gradle.properties file. I left the documentation for the new approach in the Readme and in the code itself.

@stepango stepango merged commit 26fb981 into master Jul 8, 2023
4 checks passed
@stepango stepango deleted the tonykolomeytsev/fix-includer-behavior branch July 8, 2023 22:50
@@ -58,7 +58,7 @@ dependencyResolutionManagement {
addPlugin("tools.forma.demo:dependencies", "0.0.1")
addPlugin(
"com.google.devtools.ksp",
"1.8.10-1.0.9",
"$embeddedKotlinVersion-1.0.9",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it would work. For example, after updating to gradle-8.2 embedded kotlin version would be 1.8.20, but there is NO ksp version 1.8.20-1.0.9: https://github.com/google/ksp/releases

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

3 participants