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 support for Typesafe Project Accessors #1189

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ivanalvarado
Copy link
Contributor

Description

Usage

dependencyAnalysis {
   projectProperties {
     // (Optional) Specify whether to print advice in typesafe-project-accessors format
     // Example: project(":foo:bar:baz") vs. projects.foo.bar.baz
     useTypesafeProjectAccessors(true)
   }
 }

Example

If this property is set to true, the console report will output:

Existing dependencies which should be modified to be as indicated:	
  implementation projects.account.impl (was api)
  implementation projects.account.public (was api)	

instead of

Existing dependencies which should be modified to be as indicated:	
  implementation project(':account:impl') (was api)	
  implementation project(':account:public') (was api)

Copy link
Owner

@autonomousapps autonomousapps left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

I feel like this value should simply default to true if the project is using typesafe project accessors. I don't know why we'd want it configurable. Is there a way to just detect that and enable this output automatically in that case?

Comment on lines +61 to +64
/** Customize project properties. See [ProjectHandler] for more information. */
fun projectProperties(action: Action<ProjectHandler>) {
action.execute(projectHandler)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure about this method name or using this as the access point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't 100% confident on the name either. I mainly didn't want to piggy back on one of the existing handlers as it didn't really "fit" into any of them. I'm in between "GradleFeature" and "ProjectFeature" because in order to enable this "feature", one has to specify:

enableFeaturePreview("TYPESAFE_PROJECT_ACCESSORS")

I feel like this value should simply default to true if the project is using typesafe project accessors. I don't know why we'd want it configurable. Is there a way to just detect that and enable this output automatically in that case?

My initial goal was to autodetect if the project was using type-safe project accessors. I dug into the Settings object API, but could not find any indicators on whether or not this feature was enabled. I asked in Gradle Community Slack and the feedback was "there is no way to detect". The suggestion was to expose a configurable boolean through the Extension. (Wish I could grab a reference to the convo, but the thread is older than 90 days and not discoverable anymore 😞)

Copy link
Owner

Choose a reason for hiding this comment

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

I would be ok with exposing this new property in the top-level DSL object (dependencyAnalysis).

If you file (or discover) an issue for auto-detecting this, please link it to this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Gradle has confirmed to me directly that there is no API to detect this. Nevertheless I think it would be a better user experience if it were automatic. How do you feel about parsing the settings script with regex, looking for that specific line that enables the feature?

I also wonder if we can inspect the buildscript classloader for a specific jar or class file, where these accessors would live?

Copy link
Owner

Choose a reason for hiding this comment

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

I've thought about this some more and decided that, since this is an experimental gradle feature, I am in favor of the current approach of requiring users to opt-in. I don't want to tie DAGP to some hacky approach to detecting it that is likely to change at any time.

That said, let's move the property to the top level dependencyAnalysis extension. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Yeah, my worry was that Gradle would remove the need for the enableFeaturePreview syntax once typesafe project accessors were promoted to stable.

I'll go ahead and move the property to top level.

Comment on lines +46 to +48
fun projectProperties(action: Action<ProjectHandler>) {
action.execute(projectHandler)
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same comment as earlier, unsure about this.

}

private fun String.kebabToCamelCase(): String {
val pattern = "-[a-z]".toRegex()
Copy link
Owner

Choose a reason for hiding this comment

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

Does this regex work in all possible cases? Does gradle only permit ASCII characters or is a wider character range available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will follow up on this.

src/main/kotlin/com/autonomousapps/tasks/RewriteTask.kt Outdated Show resolved Hide resolved
@alllex
Copy link

alllex commented Jun 25, 2024

Ideally, the error should provide the suggestion compatible with the corresponding build script DSL, i.e. Groovy vs Kotlin.

For Kotlin DSL it should be:

Existing dependencies which should be modified to be as indicated:	
  implementation(projects.account.impl) (was api)
  implementation(projects.account.public) (was api)	

@alllex
Copy link

alllex commented Jun 25, 2024

Given that it's not possible to detect whether type-safe accessors are enabled and that it might not be easy to detect the correct DSL, I suggest adding those as formatting options in the plugin configuration.

This will already be immediately useful for projects using the plugin because the errors will provide a better UX.

On that note, even if the type-safe accessors become stable and are ON by default, the users might still prefer the stringy project coordinates. This means they would still prefer to see the errors displayed in the format matching theirs. Thus, the plugin would still require configurability in this aspect.

@autonomousapps
Copy link
Owner

Given that it's not possible to detect whether type-safe accessors are enabled and that it might not be easy to detect the correct DSL, I suggest adding those as formatting options in the plugin configuration.

The plugin currently does detect whether the project is using Groovy or Kotlin based on the file extension (.gradle or .gradle.kts), and formats the output accordingly. All the tests here use Groovy DSL for historical reasons.

@eygraber
Copy link

Is there anything that still needs to be done here?

@autonomousapps
Copy link
Owner

At least it needs to be rebased and the conflicts fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants