Revise unobvious defaults #68
Conversation
WalkthroughThis set of changes updates the Bukkit Gradle plugin to modernize dependency management, configuration, and server core support. The default Bukkit API version is no longer set automatically and must be specified explicitly. Support for Spigot and other cores is removed, with PaperMC as the only supported core. Dependency resolution now uses version placeholders and group ID substitution rules, and users must add repositories manually. The plugin.yml generation process is updated to use the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Gradle as Gradle Project
participant BukkitExt as BukkitExtension
participant DevServer as DevServerFeature
participant RunPaper as RunPaperPlugin
User->>Gradle: Applies BukkitGradlePlugin
Gradle->>BukkitExt: Creates BukkitExtension (requires explicit apiVersion)
Gradle->>DevServer: Configures DevServerFeature with BukkitExtension
DevServer->>RunPaper: Applies RunPaperPlugin
DevServer->>Gradle: Registers RunServer & PrepareServer tasks
DevServer->>Gradle: Registers IDE Run Config task
User->>Gradle: Adds dependencies with {bukkit.apiVersion} placeholder
Gradle->>BukkitExt: Resolves apiVersion and substitutes in dependencies
Gradle->>Gradle: Resolves PaperMC groupId as needed
Gradle->>Gradle: Builds and runs server using configured tasks
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
README.md (1)
107-107: Minor: Add comma after "That's it!"There should be a comma after "That's it!" for better readability.
-That's it! +That's it!,🧰 Tools
🪛 LanguageTool
[uncategorized] ~107-~107: Possible missing comma found.
Context: ...c() } ``` That's it! During the plugin compilationplugin.ymlwill be generated with the...(AI_HYDRA_LEO_MISSING_COMMA)
src/main/kotlin/BukkitGradlePlugin.kt (1)
44-46: Prefer usingconventionover hard‑wiring the compiler encodingHard‑setting the encoding with
options.encoding = "UTF-8"overrides any subsequent user configuration.
Switching to theconventionAPI will still default to UTF‑8, but allows a build script to override it later if needed.- options.encoding = "UTF-8" + // keep UTF‑8 as the default, but let the user override it afterwards + options.encoding.convention("UTF-8")src/main/kotlin/dependencies/Dependencies.kt (2)
58-63: Kotlin‑DSL builds cannot call the new Groovy closures
extra["spigot"] = repositoryClosure(...)(and friends) exposes only dynamic Groovy methods (spigot(),papermc(), …).
If a consumer adopts the Kotlin DSL (build.gradle.kts), these symbols are not available at compile time, forcing them to fall back to string‑basedmaven { url = … }blocks.Consider adding typed extension functions alongside the closures so both Groovy and Kotlin builds get first‑class support, e.g.
fun RepositoryHandler.spigot(block: MavenArtifactRepository.() -> Unit = {}) = addRepo("Spigot", URL_SPIGOT, block)That keeps the ergonomic one‑liner while remaining cross‑DSL.
86-114: Minor optimisation: avoid resolving the Provider on every dependency
bukkitVersion.get()is executed insideeachDependency, i.e. once per dependency candidate.
Caching it locally avoids repeated provider resolution and string concatenations:- val bukkitVersion = bukkit.finalApiVersion - .map { "$it$BUKKIT_VERSION_SUFFIX" } + val bukkitVersionProvider = bukkit.finalApiVersion + .map { "$it$BUKKIT_VERSION_SUFFIX" } … - val version = if (requested.version == BUKKIT_VERSION_PLACEHOLDER) { - bukkitVersion.get() + val version = if (requested.version == BUKKIT_VERSION_PLACEHOLDER) { + bukkitVersionProvider.get()The change is optional, but it marginally improves configuration‑time performance on large dependency graphs.
src/test/groovy/ru/endlesscode/bukkitgradle/BukkitGradlePluginSpec.groovy (1)
10-17: Solid negative‑path test – one extra assertion would tighten itThe test correctly verifies that an exception surfaces when
apiVersionis missing.
To ensure the plugin throws your specificRuntimeException, also assert on the exception’s type, not only its message:then: def e = thrown(RuntimeException) e instanceof MissingApiVersionException // or the concrete subclass you expect e.cause.message == "Please, set 'bukkit.apiVersion' property."This will prevent future refactors from masking the intended failure type via a different wrapper exception.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.md(2 hunks)README.md(11 hunks)src/main/kotlin/BukkitExtension.kt(1 hunks)src/main/kotlin/BukkitGradlePlugin.kt(2 hunks)src/main/kotlin/dependencies/Dependencies.kt(3 hunks)src/main/kotlin/dependencies/DependenciesExtensions.kt(3 hunks)src/main/kotlin/plugin/PluginYamlFeature.kt(0 hunks)src/main/kotlin/plugin/util/MinecraftVersion.kt(2 hunks)src/main/kotlin/server/Constants.kt(0 hunks)src/main/kotlin/server/DevServerFeature.kt(1 hunks)src/main/kotlin/server/DevServerPlugin.kt(0 hunks)src/main/kotlin/server/extension/CoreType.kt(0 hunks)src/main/kotlin/server/extension/ServerConfiguration.kt(0 hunks)src/main/kotlin/server/extension/ServerConfigurationImpl.kt(1 hunks)src/main/kotlin/server/task/PrepareServer.kt(1 hunks)src/test/groovy/ru/endlesscode/bukkitgradle/BukkitGradlePluginSpec.groovy(2 hunks)src/test/groovy/ru/endlesscode/bukkitgradle/server/extension/ServerConfigurationSpec.groovy(0 hunks)
💤 Files with no reviewable changes (6)
- src/main/kotlin/plugin/PluginYamlFeature.kt
- src/main/kotlin/server/extension/ServerConfiguration.kt
- src/main/kotlin/server/extension/CoreType.kt
- src/main/kotlin/server/Constants.kt
- src/test/groovy/ru/endlesscode/bukkitgradle/server/extension/ServerConfigurationSpec.groovy
- src/main/kotlin/server/DevServerPlugin.kt
🧰 Additional context used
🧠 Learnings (1)
README.md (2)
Learnt from: osipxd
PR: EndlessCodeGroup/BukkitGradle#66
File: README.md:104-108
Timestamp: 2025-04-17T20:35:24.623Z
Learning: In Bukkit plugin.yml, the `api-version` field doesn't support patch versions for Minecraft versions lower than 1.20.5. Even if `apiVersion = "1.16.5"` is specified in configuration, it will be transformed to `api-version: '1.16'` in the generated plugin.yml file. Starting from Minecraft 1.20.5, full patch versions are supported in the api-version field.
Learnt from: osipxd
PR: EndlessCodeGroup/BukkitGradle#66
File: README.md:104-108
Timestamp: 2025-04-17T20:35:24.623Z
Learning: In Bukkit plugin.yml, the `api-version` field doesn't support patch versions for Minecraft versions lower than 1.20.5. Even if `apiVersion = "1.16.5"` is specified in configuration, it will be transformed to `api-version: '1.16'` in the generated plugin.yml file. Starting from Minecraft 1.20.5, full patch versions are supported in the api-version field.
🧬 Code Graph Analysis (1)
src/main/kotlin/dependencies/DependenciesExtensions.kt (1)
src/main/kotlin/dependencies/Dependencies.kt (1)
withBukkitVersion(82-84)
🪛 markdownlint-cli2 (0.17.2)
README.md
24-24: Heading levels should only increment by one level at a time
Expected: h2; Actual: h4
(MD001, heading-increment)
24-24: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
24-24: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
153-153: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
153-153: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
157-157: Bare URL used
null
(MD034, no-bare-urls)
158-158: Bare URL used
null
(MD034, no-bare-urls)
159-159: Bare URL used
null
(MD034, no-bare-urls)
160-160: Bare URL used
null
(MD034, no-bare-urls)
161-161: Bare URL used
null
(MD034, no-bare-urls)
162-162: Bare URL used
null
(MD034, no-bare-urls)
163-163: Bare URL used
null
(MD034, no-bare-urls)
164-164: Bare URL used
null
(MD034, no-bare-urls)
165-165: Bare URL used
null
(MD034, no-bare-urls)
167-167: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
344-344: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🪛 LanguageTool
README.md
[uncategorized] ~107-~107: Possible missing comma found.
Context: ...c() } ``` That's it! During the plugin compilation plugin.yml will be generated with the...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check
🔇 Additional comments (22)
src/main/kotlin/server/extension/ServerConfigurationImpl.kt (1)
19-30: Well-structured deprecation approach for server core selectionThe implementation correctly deprecates the
setCoremethod while maintaining backward compatibility. The warning message is clear and provides actionable guidance to users: remove the method call and file an issue if other server cores are needed.This change aligns with the PR objective of making configuration behavior more explicit by removing hidden defaults and simplifying the server core selection to use Paper exclusively.
src/main/kotlin/plugin/util/MinecraftVersion.kt (1)
19-21: Improved version parsing with suffix handlingThe updated parsing logic now properly handles version strings that include the Bukkit version suffix by removing it before parsing. This change supports the more explicit version handling approach in the PR and ensures consistent version parsing regardless of whether the suffix is present.
src/main/kotlin/server/task/PrepareServer.kt (2)
32-33: Method name improvement in task actionRenaming from
resolveOnlineMode()toconfigureOnlineMode()better reflects the actual purpose of the method, which is to configure the online-mode property rather than just resolve a value.
35-45: Enhanced resource management for file operationsThe implementation now uses Kotlin's
useextension function for both reading and writing to the properties file. This ensures resources are properly closed even in case of exceptions, which is a best practice for file I/O operations.src/main/kotlin/BukkitExtension.kt (2)
23-29: Removed implicit default for apiVersionThe code now explicitly requires users to set the
bukkit.apiVersionproperty by removing the default value and adding a validation check. This change directly implements one of the key objectives of the PR: making implicit defaults explicit and ensuring users make conscious configuration choices.The error message is clear and provides guidance on what needs to be done.
25-30: Well-designed provider architecture for version handlingThe introduction of separate providers for finalized API version and parsed version follows good separation of concerns. The
finalApiVersionprovider handles validation and finalization, whileparsedApiVersionhandles the parsing logic. This design maintains a clean architecture while enforcing the requirement for explicit version specification.CHANGELOG.md (4)
6-11: Good addition of dependency substitution documentationThe new section clearly explains how to use dependency substitution rules with the placeholder value
{bukkit.apiVersion}in version catalogs, which makes the dependency resolution mechanism more transparent to users.
18-21: Clear explanation of server core type removalThe changelog clearly communicates the breaking change regarding server core support, providing a good rationale and alternative (filing issues) for users who need other server cores.
35-43: Well-documented change for repository handlingThe explanation for removing implicit repository additions is thorough and includes a helpful example of how users should now explicitly add repositories. This directly addresses the PR objective of making configuration more explicit.
44-52: Clear communication about apiVersion default removalThe changelog effectively explains the removal of the implicit default for
bukkit.apiVersion, providing both the rationale ("unobvious behavior") and a clear example of how users should configure it going forward.src/main/kotlin/dependencies/DependenciesExtensions.kt (2)
1-20: Import changes align with dependency substitution modelThe updated imports and additional suppression reflect the refactoring to use placeholder versions instead of resolved versions, which aligns with the PR objective of making version defaults explicit.
49-59: Refactored dependency properties to use placeholdersGood refactoring of dependency properties to use
withBukkitVersionwhich returns a dependency notation with a placeholder that will be substituted at resolution time, making the version dependency more explicit.README.md (5)
90-93: Updated documentation to show explicit apiVersion requirementThe changes properly document that users must now explicitly set
apiVersion, aligning with the PR objective of eliminating implicit defaults.
96-103: Updated example to reflect PaperMC as the only supported coreThe example code has been updated to use
paperApiand explicitly add thepapermc()repository, consistent with the changes to remove Spigot support.
155-166: Improved repository documentation formattingThe repositories table is now better formatted, making it easier for users to understand which repositories are available.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
157-157: Bare URL used
null(MD034, no-bare-urls)
158-158: Bare URL used
null(MD034, no-bare-urls)
159-159: Bare URL used
null(MD034, no-bare-urls)
160-160: Bare URL used
null(MD034, no-bare-urls)
161-161: Bare URL used
null(MD034, no-bare-urls)
162-162: Bare URL used
null(MD034, no-bare-urls)
163-163: Bare URL used
null(MD034, no-bare-urls)
164-164: Bare URL used
null(MD034, no-bare-urls)
165-165: Bare URL used
null(MD034, no-bare-urls)
171-180: Clear dependency documentation with repository requirementsThis section now explicitly indicates which repository is needed for each dependency, helping users understand the relationship between dependencies and repositories after the removal of implicit repository additions.
257-295: Comprehensive migration guide for 0.10.xThe migration guide clearly outlines all the steps required to upgrade from 0.10.x, addressing each breaking change including the need to explicitly set
apiVersion, remove server core selection, and add repositories manually.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
257-257: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
src/main/kotlin/server/DevServerFeature.kt (4)
18-46: Well-implemented server feature configurationThe dev server feature is well-implemented with proper use of Gradle's provider API for deferred configuration. The server version falls back to the explicitly configured
bukkit.finalApiVersion, aligning with the PR objective of eliminating implicit defaults.
48-61: Good implementation of JVM toolchain configurationThe method properly configures the Java launcher for the server based on the project's Java toolchain and the minimal JVM version required by the server version, ensuring compatibility.
63-70: Explicit server directory resolutionThe implementation correctly resolves the server directory from a Gradle property or a deprecated fallback property, making configuration more explicit and predictable.
72-87: Well-structured task registration methodsThe task registration methods are well-structured and follow Gradle best practices for task configuration and dependencies. The tasks are properly integrated with the RunServer task from the RunPaperPlugin.
src/main/kotlin/BukkitGradlePlugin.kt (1)
21-37: Good inline refactor & removal of silent defaultsMoving the former helper logic directly into
apply()makes the execution flow easier to follow, and the explicit call toconfigureDependencyExtensionsnicely groups all dependency‑related tweaks in one place.
I see no functional or stylistic issues here – well done.
osipxd
force-pushed
the
breaking-changes-before-release
branch
from
0e50073 to
0ea83c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CHANGELOG.md (2)
6-11: Clear documentation of dependency substitution rulesThe changelog effectively explains the new dependency substitution feature with a helpful example. The use of a code block to illustrate the version catalog syntax is particularly useful.
Minor grammatical suggestion: Add a comma after "In version catalogs" for better readability.
- In version catalogs placeholder value `{bukkit.version}` can be used, and it will be replaced with the actual version: + In version catalogs, placeholder value `{bukkit.apiVersion}` can be used, and it will be replaced with the actual version:🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...bstituting bukkit version. In version catalogs placeholder value{bukkit.version}ca...(AI_HYDRA_LEO_MISSING_COMMA)
44-52: Clear explanation of apiVersion default removalThe explanation of removing the implicit default for
bukkit.apiVersionaligns with the PR objective of eliminating unobvious defaults. The example code snippet helps users understand how to adapt to this change.Minor suggestion: Consider adding "the" before "default" for better readability.
- Don't set default `bukkit.apiVersion`. + Don't set the default `bukkit.apiVersion`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ... ``` - Breaking change! Don't set defaultbukkit.apiVersion. It was implicitl...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
README.md (1)
178-180: Minor markdown formatting issue in footnotesThere are extra spaces after the backslashes in the footnotes which might affect rendering.
-\* Spigot is available in `mavenLocal()` only if you've built it locally using [Spigot BuildTools][buildtools]. \ -\*\* Bukkit should be built locally. However, some versions are available in `spigot()` repository. \ +\* Spigot is available in `mavenLocal()` only if you've built it locally using [Spigot BuildTools][buildtools].\ +\*\* Bukkit should be built locally. However, some versions are available in `spigot()` repository.\
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
CHANGELOG.md(2 hunks)README.md(11 hunks)src/main/kotlin/BukkitExtension.kt(1 hunks)src/main/kotlin/BukkitGradlePlugin.kt(2 hunks)src/main/kotlin/dependencies/Dependencies.kt(3 hunks)src/main/kotlin/dependencies/DependenciesExtensions.kt(3 hunks)src/main/kotlin/extensions/Provider.kt(0 hunks)src/main/kotlin/plugin/PluginYamlFeature.kt(0 hunks)src/main/kotlin/plugin/util/MinecraftVersion.kt(2 hunks)src/main/kotlin/server/Constants.kt(0 hunks)src/main/kotlin/server/DevServerFeature.kt(1 hunks)src/test/groovy/ru/endlesscode/bukkitgradle/BukkitGradlePluginSpec.groovy(2 hunks)
💤 Files with no reviewable changes (3)
- src/main/kotlin/plugin/PluginYamlFeature.kt
- src/main/kotlin/server/Constants.kt
- src/main/kotlin/extensions/Provider.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/kotlin/plugin/util/MinecraftVersion.kt
- src/main/kotlin/BukkitExtension.kt
- src/main/kotlin/dependencies/DependenciesExtensions.kt
- src/main/kotlin/BukkitGradlePlugin.kt
- src/test/groovy/ru/endlesscode/bukkitgradle/BukkitGradlePluginSpec.groovy
- src/main/kotlin/server/DevServerFeature.kt
🧰 Additional context used
🧠 Learnings (1)
README.md (2)
Learnt from: osipxd
PR: EndlessCodeGroup/BukkitGradle#66
File: README.md:104-108
Timestamp: 2025-04-17T20:35:24.623Z
Learning: In Bukkit plugin.yml, the `api-version` field doesn't support patch versions for Minecraft versions lower than 1.20.5. Even if `apiVersion = "1.16.5"` is specified in configuration, it will be transformed to `api-version: '1.16'` in the generated plugin.yml file. Starting from Minecraft 1.20.5, full patch versions are supported in the api-version field.
Learnt from: osipxd
PR: EndlessCodeGroup/BukkitGradle#66
File: README.md:104-108
Timestamp: 2025-04-17T20:35:24.623Z
Learning: In Bukkit plugin.yml, the `api-version` field doesn't support patch versions for Minecraft versions lower than 1.20.5. Even if `apiVersion = "1.16.5"` is specified in configuration, it will be transformed to `api-version: '1.16'` in the generated plugin.yml file. Starting from Minecraft 1.20.5, full patch versions are supported in the api-version field.
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...bstituting bukkit version. In version catalogs placeholder value {bukkit.version} ca...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ... ``` - Breaking change! Don't set default bukkit.apiVersion. It was implicitl...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
README.md
[uncategorized] ~107-~107: Possible missing comma found.
Context: ...c() } ``` That's it! During the plugin compilation plugin.yml will be generated with the...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
README.md
24-24: Heading levels should only increment by one level at a time
Expected: h2; Actual: h4
(MD001, heading-increment)
24-24: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
24-24: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
153-153: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
153-153: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
157-157: Bare URL used
null
(MD034, no-bare-urls)
158-158: Bare URL used
null
(MD034, no-bare-urls)
159-159: Bare URL used
null
(MD034, no-bare-urls)
160-160: Bare URL used
null
(MD034, no-bare-urls)
161-161: Bare URL used
null
(MD034, no-bare-urls)
162-162: Bare URL used
null
(MD034, no-bare-urls)
163-163: Bare URL used
null
(MD034, no-bare-urls)
164-164: Bare URL used
null
(MD034, no-bare-urls)
165-165: Bare URL used
null
(MD034, no-bare-urls)
167-167: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
344-344: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (15)
src/main/kotlin/dependencies/Dependencies.kt (7)
29-30: Well-structured version placeholdersThe use of constants for version placeholders and suffixes improves maintainability and makes the code more readable. This aligns with the PR objective of making version configuration more explicit.
32-34: Good use of constants for group IDsDefining constants for both old and new Paper group IDs centralizes this information and makes it easier to maintain. This approach supports the dynamic group ID substitution based on Minecraft version.
39-44: Clean separation of concernsThe
configureDependencyExtensionsfunction properly separates the extension setup from resolution strategy configuration, making the code more modular and maintainable.
46-56: Repository extensions no longer automatically appliedThis implementation aligns with the PR objective of removing implicit repository additions. Users now must explicitly add repositories, making the configuration behavior more predictable.
58-63: Dependencies use version placeholdersThe dependency declarations now use placeholders instead of concrete versions, which is a cleaner approach and supports the resolution strategy.
82-84: Simple and clear dependency string generatorThis function elegantly generates dependency notation with the version placeholder, supporting the goal of making version configuration more explicit.
86-116: Elegant resolution strategy implementationThe resolution strategy effectively handles both version placeholder substitution and group ID rewriting based on Minecraft version. This implementation properly fixes issue #59 by ensuring version resolution happens during the dependency resolution phase rather than the configuration phase.
CHANGELOG.md (2)
18-21: Clear explanation of server core support changesThe changelog clearly communicates the removal of Spigot support and provides guidance for users who need other server cores. This transparency helps users understand the rationale behind the change.
35-43: Well-documented breaking change for repository handlingThe changelog clearly explains the rationale for requiring explicit repository declarations and provides a helpful example. This is essential information for users migrating from previous versions.
README.md (6)
90-93: Updated example with explicit apiVersionThe example now correctly shows the explicit setting of
apiVersion, aligning with the PR objective to eliminate implicit defaults.
101-103: Example shows explicit repository declarationThis example demonstrates the new requirement to explicitly declare repositories, which is a key change in the PR.
110-110: Version representation in plugin.ymlBased on the retrieved learnings, for Minecraft versions prior to 1.20.5, the
api-versionfield would omit the patch version, but this example shows the full version. This is correct for 1.21.5+ as the learnings indicate full patch versions are supported starting from 1.20.5.
276-281: Clear migration instructions for apiVersionThe migration guide clearly explains the need to explicitly set
apiVersion, which is essential for users upgrading from previous versions.
283-285: Clear guidance on server core changesThe migration guide effectively communicates the removal of server core selection and the standardization on PaperMC, which is a significant change for users.
288-294: Comprehensive repository migration guidanceThis section provides clear instructions on the need to explicitly add repositories, with helpful examples that will ease the transition for users.
osipxd
force-pushed
the
breaking-changes-before-release
branch
from
0ea83c5 to
1ad7b08
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
CHANGELOG.md (2)
6-11: Good addition of dependency substitution rulesThe addition of dependency substitution rules for handling paper groupId and version placeholders is well-documented. This feature will make dependency management more flexible.
Consider adding a comma after "version." in line 7 for better readability.
- In version catalogs placeholder value `{bukkit.version}` can be used, and it will be replaced with the actual version: + In version catalogs, placeholder value `{bukkit.version}` can be used, and it will be replaced with the actual version:🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...bstituting bukkit version. In version catalogs placeholder value{bukkit.version}ca...(AI_HYDRA_LEO_MISSING_COMMA)
44-52: Good explanation of mandatory apiVersionThe explanation for removing the implicit default for
bukkit.apiVersionis clear and provides a proper code example. This change improves configuration transparency.Consider adding "the" before "default" in line 45 for better readability.
-Don't set default `bukkit.apiVersion`. +Don't set the default `bukkit.apiVersion`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ... ``` - Breaking change! Don't set defaultbukkit.apiVersion. It was implicitl...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
README.md (2)
106-115: Updated plugin.yml exampleThe example output for generated plugin.yml has been updated to show the proper format for the api-version field. This is accurate and aligns with the learned information about API version formatting.
Consider adding a comma after "That's it!" for better readability.
-That's it! -During the plugin compilation `plugin.yml` will be generated with the following content: +That's it! +During the plugin compilation, `plugin.yml` will be generated with the following content:🧰 Tools
🪛 LanguageTool
[uncategorized] ~107-~107: A comma might be missing here.
Context: ...c() } ``` That's it! During the plugin compilationplugin.ymlwill be generated with the...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
171-180: Updated dependency table with clarified repository requirementsThe dependency table now clearly indicates which repository is required for each dependency, and provides helpful footnotes about local builds. This improves usability.
Consider adding "the" before "spigot()" in line 179 for better readability.
-\*\* Bukkit should be built locally. However, some versions are available in `spigot()` repository. \ +\*\* Bukkit should be built locally. However, some versions are available in the `spigot()` repository. \🧰 Tools
🪛 LanguageTool
[uncategorized] ~179-~179: You might be missing the article “the” here.
Context: ...However, some versions are available inspigot()repository. \ *** `$apiVer...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(2 hunks)README.md(11 hunks)src/main/kotlin/BukkitExtension.kt(1 hunks)src/main/kotlin/server/Constants.kt(0 hunks)src/test/groovy/ru/endlesscode/bukkitgradle/BukkitGradlePluginSpec.groovy(2 hunks)
💤 Files with no reviewable changes (1)
- src/main/kotlin/server/Constants.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/BukkitExtension.kt
🧰 Additional context used
🧠 Learnings (1)
README.md (2)
Learnt from: osipxd
PR: EndlessCodeGroup/BukkitGradle#66
File: README.md:104-108
Timestamp: 2025-04-17T20:35:24.623Z
Learning: In Bukkit plugin.yml, the `api-version` field doesn't support patch versions for Minecraft versions lower than 1.20.5. Even if `apiVersion = "1.16.5"` is specified in configuration, it will be transformed to `api-version: '1.16'` in the generated plugin.yml file. Starting from Minecraft 1.20.5, full patch versions are supported in the api-version field.
Learnt from: osipxd
PR: EndlessCodeGroup/BukkitGradle#66
File: README.md:104-108
Timestamp: 2025-04-17T20:35:24.623Z
Learning: In Bukkit plugin.yml, the `api-version` field doesn't support patch versions for Minecraft versions lower than 1.20.5. Even if `apiVersion = "1.16.5"` is specified in configuration, it will be transformed to `api-version: '1.16'` in the generated plugin.yml file. Starting from Minecraft 1.20.5, full patch versions are supported in the api-version field.
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~7-~7: Possible missing comma found.
Context: ...bstituting bukkit version. In version catalogs placeholder value {bukkit.version} ca...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~45-~45: You might be missing the article “the” here.
Context: ... ``` - Breaking change! Don't set default bukkit.apiVersion. It was implicitl...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
README.md
[uncategorized] ~107-~107: A comma might be missing here.
Context: ...c() } ``` That's it! During the plugin compilation plugin.yml will be generated with the...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~179-~179: You might be missing the article “the” here.
Context: ...However, some versions are available in spigot() repository. \ *** `$apiVer...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 markdownlint-cli2 (0.17.2)
README.md
24-24: Heading levels should only increment by one level at a time
Expected: h2; Actual: h4
(MD001, heading-increment)
24-24: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
24-24: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
153-153: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
153-153: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
157-157: Bare URL used
null
(MD034, no-bare-urls)
158-158: Bare URL used
null
(MD034, no-bare-urls)
159-159: Bare URL used
null
(MD034, no-bare-urls)
160-160: Bare URL used
null
(MD034, no-bare-urls)
161-161: Bare URL used
null
(MD034, no-bare-urls)
162-162: Bare URL used
null
(MD034, no-bare-urls)
163-163: Bare URL used
null
(MD034, no-bare-urls)
164-164: Bare URL used
null
(MD034, no-bare-urls)
165-165: Bare URL used
null
(MD034, no-bare-urls)
167-167: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
344-344: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (10)
CHANGELOG.md (2)
18-21: Clear communication about dropping Spigot supportGood job on clearly communicating that Spigot is no longer supported and providing guidance for users who may need other server cores.
35-43: Excellent explanation of repository changesThis section provides a clear explanation of why repositories must now be added explicitly, with a helpful example. This change makes the behavior more predictable and explicit.
README.md (3)
91-103: Updated example with explicit apiVersionThe quick start example has been appropriately updated to use the latest API version (1.21.5) and require explicit setting of
apiVersion. This aligns perfectly with the PR objectives of making defaults explicit.
155-165: Improved repository table formattingThe repository table has been reformatted for better readability and consistency. The new markdown table format is more organized and easier to read.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
157-157: Bare URL used
null(MD034, no-bare-urls)
158-158: Bare URL used
null(MD034, no-bare-urls)
159-159: Bare URL used
null(MD034, no-bare-urls)
160-160: Bare URL used
null(MD034, no-bare-urls)
161-161: Bare URL used
null(MD034, no-bare-urls)
162-162: Bare URL used
null(MD034, no-bare-urls)
163-163: Bare URL used
null(MD034, no-bare-urls)
164-164: Bare URL used
null(MD034, no-bare-urls)
165-165: Bare URL used
null(MD034, no-bare-urls)
276-294: Clear migration instructionsThe migration guide clearly outlines the steps needed to update from 0.10.x, particularly highlighting the need to explicitly specify
apiVersionand add repositories manually. This is essential documentation for a breaking change.src/test/groovy/ru/endlesscode/bukkitgradle/BukkitGradlePluginSpec.groovy (5)
10-17: Good test for mandatory apiVersion requirementThis new test correctly verifies that an appropriate error is shown when attempting to access the toolchain language version without setting
bukkit.apiVersion. The error message is clear and actionable.
44-50: Updated test for version placeholderThe test has been updated to verify that the bukkit extension returns a dependency with the
{bukkit.apiVersion}placeholder instead of a hardcoded default version. This correctly validates the removal of implicit defaults.
56-67: Improved dependency resolution testingThe test has been updated to verify actual resolved dependencies rather than string outputs. This is a more robust approach to testing the dependency resolution mechanism, including group ID substitution for PaperMC.
74-89: Good test for group ID substitutionThis test properly verifies that the group ID is corrected when using Paper with a new version but the old group ID, which is a key feature of the dependency substitution rules.
108-112: Useful helper method for testingThe
resolvedDependency()method is a good addition that centralizes the logic for retrieving resolved artifact identifiers, making the tests more maintainable.
This PR aims to fix unibvious or unchangeable defaults.
mavenCentraland other repositories automaticallyapiVersion, require it to be set explicitlyapiVersionduring configuration if dependency extensions was used (proper fix for apiVersion attribute not being respected #59)