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 a new global plugin API. #311

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Add a new global plugin API. #311

merged 3 commits into from
Nov 29, 2023

Conversation

Laimiux
Copy link
Collaborator

@Laimiux Laimiux commented Nov 28, 2023

What

Enable global introspection via a plugins API.

@carrotkite
Copy link

carrotkite commented Nov 28, 2023

JaCoCo Code Coverage 79.53% ✅

Class Covered Meta Status
com/instacart/formula/coroutines/FlowRuntime 100% 0%
com/instacart/formula/internal/ListInspector 81% 0%
com/instacart/formula/rxjava3/RxJavaRuntime 100% 0%
com/instacart/formula/FormulaPlugins 100% 0%

Generated by 🚫 Danger

Copy link
Collaborator

@alexanderbezverhni alexanderbezverhni left a comment

Choose a reason for hiding this comment

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

LGTM, only few nits

Comment on lines 11 to 13
for (inspector in inspectors) {
inspector.onFormulaStarted(formulaType)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe introduce private fun forEveryInspector(block: Inspector.() -> Unit) to make this class less verbose and a little more kt idiomatic?

forEveryInspector { onFormulaStarted(formulaType) }

Comment on lines +1257 to +1262
val globalInspector = TestInspector()
FormulaPlugins.setPlugin(object : Plugin {
override fun inspector(type: KClass<*>): Inspector {
return globalInspector
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: should we have test cases for any possible (global, local) permutation, not only when both are not null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have some tests for local only, added a new test for global only.

@Laimiux Laimiux merged commit d9b5c98 into master Nov 29, 2023
5 checks passed
@Laimiux Laimiux deleted the laimonas/plugins-api branch November 29, 2023 18:14
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.

3 participants