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

Give advice to change to runtimeOnly where appropriate #646

Merged

Conversation

jjohannes
Copy link
Collaborator

If an advice was computed to change a declaration from 'implementation'/'api' to 'runtimeOnly', this is indeed correct
information. The dependency is not required at compile time. But it might be required at runtime.

It is not possible for this plugin to make a clear statement about if it is actually needed at runtime. When getting such an advice, the user might also remove the dependency completely. The 'reason' feature will give more insight on why the dependency might be needed at runtime. The following cases are covered:

  • Contains Service Loader declarations
  • Contains Security Provider declarations
  • Contains Content Provider or Service declarations (Android)
  • Contains Android linters (Lint-Registry entry)

Fixes #387

If an advice was computed to change a declaration from
'implementation'/'api' to 'runtimeOnly', this is indeed correct
information. The dependency is *not* required at compile time.
But it *might* be required at runtime.

It is not possible for this plugin to make a clear statement about
if it is actually needed at runtime. When getting such an advice, the
user might also remove the dependency completely. The 'reason' feature
will give more insight on why the dependency *might* be needed at
runtime. The following cases are covered:

- Contains Service Loader declarations
- Contains Security Provider declarations
- Contains Content Provider or Service declarations (Android)
- Contains Android linters (Lint-Registry entry)
@autonomousapps
Copy link
Owner

I think I'm pretty happy with this. I wonder if we should add this kind of advice to a new section of the report, though, since I know there's a large category of users that will find it confusing. We already do this for compileOnly, here for example.

But I'm not entirely convinced it's necessary -- I say this to provoke discussion. Thoughts?

@jjohannes
Copy link
Collaborator Author

jjohannes commented Apr 22, 2022

I think the separate section is a great idea. Because then the report can clarify that you should change something, but that you can choose between:

  1. Change to runtimeOnly
  2. Remove completely

How about this?

Dependencies which should be removed or changed to runtime-only:
  runtimeOnly("com.fasterxml.jackson.dataformat:jackson-dataformat-xml:2.13.2") (was implementation)
  runtimeOnly("org.apache.logging.log4j:log4j-api:2.17.2") (was implementation)

This makes it clear that you have the choice between:

  1. Change dependency to runtimeOnly
  2. Remove dependency completely
@autonomousapps
Copy link
Owner

How about this?

Yes! That looks great. Thanks :)

@autonomousapps
Copy link
Owner

@jjohannes the next thing we need is an update to IssueHandler so permit filtering out runtimeOnly advice. It'll be a lot of copy-pasta from the existing implementation to wire it through. I'm going to merge this, but would you mind doing that in a follow-up PR?

@autonomousapps autonomousapps merged commit 2eae0e4 into autonomousapps:main Apr 22, 2022
@jjohannes
Copy link
Collaborator Author

Thanks for integrating this!

next thing we need is an update to IssueHandler so permit filtering out runtimeOnly advice.

I wasn't aware. I will have a look and create a follow-up PR.

@jjohannes jjohannes deleted the advice-change-to-runtimeOnly branch April 25, 2022 08:09
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.

Should advise to change dependencies to runtimeOnly when that makes sense
2 participants