Align repository with DPC conventions#196
Conversation
…conventions) Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com> Agent-Logs-Url: https://github.com/Dans-Plugins/Currencies/sessions/da4f00c4-c4a0-4ccf-b87f-c3f6dcc588c4
There was a problem hiding this comment.
Pull request overview
Aligns the repository’s documentation and GitHub automation with the DPC (Dans Plugins Community) conventions, focusing on standardized README structure, contributor guidance, and CI/release workflows (no intended plugin runtime behavior changes).
Changes:
- Reworked
README.mdto follow the convention-driven section structure and link to local documentation. - Added core documentation files (
USER_GUIDE.md,COMMANDS.md,CONFIG.md,CHANGELOG.md,CONTRIBUTING.md). - Added GitHub Actions workflows for CI builds and release artifact publishing, plus Copilot repository instructions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Reorders/expands README sections and updates links to match org/conventions. |
| USER_GUIDE.md | Adds end-user setup and common usage scenarios + permissions table. |
| COMMANDS.md | Adds a commands reference for /currency and /coinpurse. |
| CONFIG.md | Documents config.yml keys with defaults/examples. |
| CHANGELOG.md | Seeds a Keep a Changelog–style changelog. |
| CONTRIBUTING.md | Adds contribution workflow, requirements, and testing guidance. |
| .github/workflows/build.yml | Adds CI build on push/PR for main/develop. |
| .github/workflows/release.yml | Builds and uploads JARs to GitHub Releases on release creation. |
| .github/copilot-instructions.md | Adds repository-specific Copilot context and conventions link. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Use the `lang/` resource files for every user-facing string; never hard-code | ||
| messages in Kotlin. | ||
| - Follow the existing package structure when adding new classes. | ||
| - Annotate every command executor and event listener with `@Override` where applicable. |
There was a problem hiding this comment.
This instruction is inaccurate for Kotlin: Kotlin uses the override keyword for overridden members, not an @Override annotation, and Bukkit event listeners use @EventHandler on handler methods rather than @Override. Updating/removing this bullet will prevent Copilot from introducing non-compiling Kotlin code.
| - Annotate every command executor and event listener with `@Override` where applicable. | |
| - In Kotlin, use the `override` keyword for overridden members, and annotate Bukkit event listener methods with `@EventHandler`. |
| - name: Upload JAR to release | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| files: build/libs/*.jar |
There was a problem hiding this comment.
The release workflow uploads build/libs/*.jar, which will likely include both the plain JAR and the shaded shadowJar output (e.g., *-all.jar). Attaching multiple artifacts can confuse users and may publish a non-runnable (non-shaded) JAR. Consider building/uploading only the intended distributable (typically the Shadow JAR) and/or tightening the glob to the expected filename pattern.
| files: build/libs/*.jar | |
| files: build/libs/*-all.jar |
|
|
||
| ### Language Files | ||
|
|
||
| Update `src/main/resources/lang/` for any user-facing string changes. |
There was a problem hiding this comment.
This repo currently doesn’t contain src/main/resources/lang/ (only plugin.yml and config.yml exist under src/main/resources/). This instruction will send contributors looking for a non-existent path; either add the expected lang/ structure or update the doc to describe how user-facing strings are managed in this project today.
| Update `src/main/resources/lang/` for any user-facing string changes. | |
| For any user-facing string changes, update the appropriate resource files under `src/main/resources/` (for example, `plugin.yml` or `config.yml`). If you add a new localization or language file structure, document it in your pull request. |
| - `src/main/resources/` – `plugin.yml`, `config.yml`, and `lang/` files | ||
| - `src/test/kotlin/` – Unit tests | ||
| - `.github/workflows/` – CI and release workflows | ||
|
|
||
| ## Coding Conventions | ||
|
|
||
| - Use the `lang/` resource files for every user-facing string; never hard-code | ||
| messages in Kotlin. |
There was a problem hiding this comment.
This instruction says to put every user-facing string in src/main/resources/lang/, but the repository currently has no lang/ resources directory and the codebase uses hard-coded sendMessage(...) strings. Either add the lang/ system/files as described or reword this guidance (e.g., “prefer” / “when introducing new messages…”) so it matches the current project structure and avoids misleading future changes.
| - `src/main/resources/` – `plugin.yml`, `config.yml`, and `lang/` files | |
| - `src/test/kotlin/` – Unit tests | |
| - `.github/workflows/` – CI and release workflows | |
| ## Coding Conventions | |
| - Use the `lang/` resource files for every user-facing string; never hard-code | |
| messages in Kotlin. | |
| - `src/main/resources/` – `plugin.yml`, `config.yml`, and other resource files (e.g., `config/`, `lang/` if present) | |
| - `src/test/kotlin/` – Unit tests | |
| - `.github/workflows/` – CI and release workflows | |
| ## Coding Conventions | |
| - Prefer using `lang/` resource files for user-facing strings. When introducing new | |
| messages, avoid hard-coding them in Kotlin where a suitable `lang/` resource | |
| structure already exists in this project. |
| ## Project Status | ||
|
|
||
| This project is in active development. | ||
|
|
||
| ### bStats | ||
| You can view the bStats page for this plugin [here](https://bstats.org/plugin/bukkit/Currencies/12810). | ||
|
|
||
| You can view the bStats page for the plugin [here](https://bstats.org/plugin/bukkit/Currencies/12810). | ||
|
|
||
| ## Roadmap | ||
|
|
||
| - [Known Bugs](https://github.com/Dans-Plugins/Currencies/issues?q=is%3Aopen+is%3Aissue+label%3Abug) | ||
| - [Planned Features](https://github.com/Dans-Plugins/Currencies/issues?q=is%3Aopen+is%3Aissue+label%3AEpic) | ||
| - [Planned Improvements](https://github.com/Dans-Plugins/Currencies/issues?q=is%3Aopen+is%3Aissue+label%3Aimprovement) | ||
|
|
||
| ## Changelog | ||
|
|
||
| - [CHANGELOG.md](CHANGELOG.md) |
There was a problem hiding this comment.
README section order appears to conflict with the stated required sequence (… → License → Project Status). ## Roadmap and ## Changelog are placed after ## Project Status, which means Project Status is no longer the final top-level section. Consider moving Roadmap/Changelog earlier (e.g., before Project Status) or folding them into Project Status as subsections to keep the required order intact.
| For manual testing, start a local Spigot server: | ||
|
|
||
| docker compose up | ||
|
|
There was a problem hiding this comment.
The manual test command here (docker compose up) doesn’t match the repository’s documented/dev scripts (./up.sh uses docker compose up --build -d, and ./down.sh exists for stopping). To avoid contributors running a different workflow than intended, align this section with the scripts (or document both explicitly, including how to stop the server).
| For manual testing, start a local Spigot server: | |
| docker compose up | |
| For manual testing, start a local Spigot server using the helper script: | |
| ./up.sh | |
| To stop the local Spigot server when you're done testing: | |
| ./down.sh |
| ### /currency info \<currency\> | ||
|
|
||
| **Description:** View information about a specific currency. | ||
| **Permission:** `currencies.list` |
There was a problem hiding this comment.
/currency info is documented as requiring currencies.list, but the implementation (CurrencyInfoCommand) does not perform any permission check (and there is no dedicated permission node in plugin.yml). Update the command reference to reflect the actual permission behavior (e.g., no permission required, or add the permission check in code if it’s intended to be restricted).
| **Permission:** `currencies.list` | |
| **Permission:** None (available to all players) |
| ### /currency rename \<currency\> \<new-name\> | ||
|
|
||
| **Description:** Rename an existing currency. Requires the appropriate faction permission or `currencies.force.rename`. | ||
| **Permission:** `currencies.rename` | ||
| **Usage:** `/currency rename <currency> <new-name>` | ||
| **Example:** `/currency rename GoldCoin SilverCoin` | ||
|
|
||
| --- | ||
|
|
||
| ### /currency set description \<currency\> \<description\> | ||
|
|
||
| **Description:** Set or update the description of a currency. Requires the appropriate faction permission or `currencies.force.desc`. | ||
| **Permission:** `currencies.desc` | ||
| **Usage:** `/currency set description <currency> <description>` | ||
| **Example:** `/currency set description GoldCoin "The official coin of the Northern Kingdom"` | ||
|
|
There was a problem hiding this comment.
The doc claims to be a full reference for /currency subcommands, but it doesn’t document /currency set name <currency> <new-name> (the primary subcommand; rename is just an alias in code) and it also omits the desc alias for setting descriptions. Consider adding entries for the set subcommands (and noting aliases) so the reference matches the actual command surface.
|
@copilot address comments |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com> Agent-Logs-Url: https://github.com/Dans-Plugins/Currencies/sessions/5694b649-0f2d-484a-acab-b70b9dcddf20
Addressed all review comments in e33a7ae:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - [Planned Improvements](https://github.com/dmccoystephenson/Currencies/issues?q=is%3Aopen+is%3Aissue+label%3Aimprovement) | ||
| Please fill out a bug report [here](https://github.com/Dans-Plugins/Currencies/issues/new). | ||
|
|
||
| - [Known Bugs](https://github.com/Dans-Plugins/Currencies/issues?q=is%3Aopen+is%3Aissue+label%3Abug) |
There was a problem hiding this comment.
The README links to “Known Bugs” under the Support section and then repeats the same “Known Bugs” link again in the later Roadmap section. This duplication can confuse readers about where to look; consider keeping issue links consolidated in the Roadmap (or keep them only under Support) and removing the extra copy.
| - [Known Bugs](https://github.com/Dans-Plugins/Currencies/issues?q=is%3Aopen+is%3Aissue+label%3Abug) |
|
@copilot address comments |
Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com> Agent-Logs-Url: https://github.com/Dans-Plugins/Currencies/sessions/cbcca2ed-2bb1-4d13-bbf7-cb808a2dc68d
docker compose upwith./up.sh/./down.shin CONTRIBUTING.mdOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.