-
-
Notifications
You must be signed in to change notification settings - Fork 424
Simplify code snippets executor #1310
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
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.
Code Review
The pull request simplifies the code snippet executor by introducing a DslLang enum and refactoring the CodeSnippetExtractor to use this enum. This change aims to improve the maintainability and readability of the code. The removal of several classes and interfaces also contributes to the simplification. Overall, the changes seem beneficial, but some points need attention.
Summary of Findings
- Missing Kotlin DSL support: The Kotlin DSL support is currently commented out. It's important to either fully implement it or remove the commented-out code to avoid confusion.
- Hardcoded 'api' and 'main' subprojects: The subproject names 'api' and 'main' are hardcoded in
SnippetExecutable.kt. Consider making these configurable or using constants to improve flexibility and maintainability. - Removal of executors and fixtures: The pull request removes the
SnippetExecutorinterface and related implementations likeGroovyBuildExecutorandNoopExecutor. Ensure that the new implementation covers all the necessary functionalities provided by the removed components.
Merge Readiness
The pull request introduces significant changes to the code snippet execution logic. While the simplification is a positive step, the missing Kotlin DSL support and hardcoded subproject names need to be addressed. I recommend addressing these issues before merging. I am unable to approve the pull request, and other reviewers should review and approve this code before merging.
...ationTest/kotlin/com/github/jengelman/gradle/plugins/shadow/snippet/KotlinBuildExecutable.kt
Show resolved
Hide resolved
src/integrationTest/kotlin/com/github/jengelman/gradle/plugins/shadow/DocCodeSnippetTest.kt
Show resolved
Hide resolved
src/integrationTest/kotlin/com/github/jengelman/gradle/plugins/shadow/DocCodeSnippetTest.kt
Outdated
Show resolved
Hide resolved
...rationTest/kotlin/com/github/jengelman/gradle/plugins/shadow/snippet/CodeSnippetExtractor.kt
Outdated
Show resolved
Hide resolved
...tegrationTest/kotlin/com/github/jengelman/gradle/plugins/shadow/snippet/SnippetExecutable.kt
Show resolved
Hide resolved
...tegrationTest/kotlin/com/github/jengelman/gradle/plugins/shadow/snippet/SnippetExecutable.kt
Show resolved
Hide resolved
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.
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
bfd25cc to
cdbffeb
Compare
|
CI failures will be fixed by #1306. |
Refs #1306.