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

Load external denylist entries via ServiceLoader #261

Closed
wants to merge 2 commits into from

Conversation

erawhctim
Copy link

Summary

Allow consumers to supply their own DenyListedEntrys via a ServiceLoader interface. Fixes #241.

Open to suggestions on any of the names, or if there's a preferred use of ServiceLoader (it's new to me!)

Requirements (place an x in each [ ])

The following point can be removed after setting up CI (such as Travis) with coverage reports (such as Codecov)

  • I've written tests to cover the new code and functionality included in this PR.

The following point can be removed after setting up a CLA reporting tool such as cla-assistant.io

@erawhctim
Copy link
Author

when running spotlessApply locally, I hit an error that I'm not sure what to do with. Subsequent spotless runs seemed to want to delete various files in my staging area 🤔 Have you run into this before?

> Task :spotlessKotlinExternalApply FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spotlessKotlinExternalApply'.
> java.nio.file.FileAlreadyExistsException: /Users/mitchware/Desktop/Workspace/erawhctim-slack-lints/build/spotless/spotlessKotlinExternal/slack-lint-checks/src/test/java/slack/lint/denylistedapis/TestEntriesLoader.kt -> /Users/mitchware/Desktop/Workspace/erawhctim-slack-lints/slack-lint-checks/src/test/java/slack/lint/denylistedapis/TestEntriesLoader.kt

Same issue happened with the DenyListedEntryLoader interface.

@ZacSweers
Copy link
Collaborator

For the spotless issue, just re-run it again without configuration cache. It's a weird issue with spotless that I don't really understand :/

Comment on lines +501 to +505
val loader = ServiceLoader.load(DenyListedEntryLoader::class.java)

return mutableSetOf<DenyListedEntry>().apply {
loader.iterator().forEachRemaining { addAll(it.entries) }
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to do something like ServiceLoader.load(...).iterator().toSet()

Copy link
Author

Choose a reason for hiding this comment

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

nice, the more concise the better. I'm not seeing an applicable toSet() call to use here, but I might be missing something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe .iterator().asSequence().toSet()

@@ -0,0 +1,5 @@
package slack.lint.denylistedapis

interface DenyListedEntryLoader {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind adding an experimental annotation for this? None of this is a stable API anyway, but want to make it doubly-clear here :). Can call it ExperimentalSlackLintApi

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also name this DenyListedEntryProvider and make it have a function instead (i.e. getEntries()) that takes a JavaContext. Then let's lazily load these in the element handler and provide the available context to it, so we can minimize loading (especially if there are entries provided that don't apply in certain contexts).

Copy link
Author

Choose a reason for hiding this comment

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

oh yeah sure, good call.

@ZacSweers
Copy link
Collaborator

@erawhctim are you still planning to finish this?

@ZacSweers ZacSweers closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow custom denylist entries to be used alongside the built-in ones
2 participants