-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Support ABI exclusions #158
Support ABI exclusions #158
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! I have no fundamental problems with this, although I did leave a few small comments.
The biggest thing this PR needs is a functional test verifying the new behavior. I can provide pointers to get you started more quickly if you want to tackle that.
src/main/kotlin/com/autonomousapps/internal/analyzer/DependencyAnalyzer.kt
Show resolved
Hide resolved
@@ -25,6 +25,10 @@ internal inline fun <T, R> Iterable<T>.mapToSet(transform: (T) -> R): HashSet<R> | |||
return mapTo(HashSet(collectionSizeOrDefault(10)), transform) | |||
} | |||
|
|||
internal inline fun <T, R> Iterable<T>.mapToLinkedHashSet(transform: (T) -> R): HashSet<R> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should change the pre-existing maptToSet()
function to use a LinkedHashSet
instead of adding a new method? Or, why do you prefer a linked hash set specifically here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be down for that, as that would match kotlin's conventional behavior as well (toSet()
guarantees preserved order)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for why LinkedHashSet - https://publicobject.com/2016/02/08/linkedhashmap-is-always-better-than-hashmap/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a3c0509
Addressed the inline comments, going to work on adding a test(s) now |
I wrote a pretty good unit test for the ABI checking. I can still write a functional test, but at this point it would be just to verify gradle plumbing rather than functionally correct exclusion behavior. Can take a crack if you think it's worth it, let me know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice! Love that new unit test. TIL. I'll give this a try locally and hopefully we can merge soon.
I want to talk a bit about the impact this has on generated advice. Without setting any exclusions and running buildHealth on a personal project, I will see this in the abi-dump.txt file:
and furthermore, the advice for this project suggests adding RxJava as an If I add the following: abi {
exclusions {
excludeClasses("com\\.seattleshelter\\.core\\.api\\.PetsService")
}
} then abi dump does not contain that interface any longer, and the advice no longer suggests adding RxJava2 as an Is this what you expected? If not, could you be precise with what your goal is? One interesting thing about this is that this plugin deals primarily in resolved dependencies. This new DSL operates at a different level (that of classes), which has an indirect impact on the dependency advice. |
Right, in this case that's expected. The idea is that you can limit exposed APIs for things that are either optional or public-only-becuse-tey-have-to-be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will merge when CI passes.
This is intended to resolve #48 by adding a new API to extensions for
abi
handling andexclusions
configuration.The API in practice looks like this:
I've left toe-holds in for directory based parsing (i.e.
build/
), but that will take more work as the current code looks only at class files and not where they originated from.No tests yet as I want to put this up for review of the design before moving further.