Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .claude/skills/document/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Read `AGENTS.md` (root + relevant module) before making changes.
| `README.md` | End users | Usage, configuration, operations table |
| `CHANGELOG.md` | End users | Version history, breaking changes |
| `docs/TESTING.md` | Contributors / AI agents | Testing strategy, base classes, patterns, commands |
| `docs/KOTLIN.md` | Contributors / AI agents | Kotlin idioms, naming conventions, anti-patterns, KDoc |
| `AGENTS.md` (root + modules) | AI agents | Architecture, conventions, guardrails |
| `.github/CONTRIBUTING.md` | Contributors | Dev setup, code reviews |

Expand Down
33 changes: 4 additions & 29 deletions .claude/skills/refactor/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,40 +21,15 @@ Remove comments that restate code. Add comments for *why* a decision was made, e

### Meaningful Naming Over Comments

If you need a comment to explain *what* code does, rename instead:
- Booleans: `is-`/`has-`/`should-`/`can-` prefixes
- Collections: plural nouns
- Functions: verb phrases (`verifyBucketExists`, `resolveVersionId`)
- Avoid abbreviations (`bucketMetadata` not `bktMd`)
If you need a comment to explain *what* code does, rename instead. See **[docs/KOTLIN.md](../../../docs/KOTLIN.md)** for naming conventions.

### Idiomatic Kotlin
### Idiomatic Kotlin & Common Anti-Patterns

- **`.let`/`.also`**: Use when they improve readability, not gratuitously
- **Expression bodies**: For single-expression functions
- **Null safety**: `?.`, `?:` over `if (x != null)` checks
- **Named `it`**: Always name in nested or non-trivial lambdas
- **`when`**: Over `if-else` chains with 3+ branches
- **Early returns**: Flatten deeply nested code
- **Extract functions**: Break up methods longer than ~30 lines

### Common Anti-Patterns to Fix

| Anti-Pattern | Refactor To |
|---|---|
| `if (x != null) { x.foo() }` | `x?.foo()` |
| `if (x == null) throw ...` | `x ?: throw ...` or `requireNotNull(x)` |
| `list.size == 0` / `list.size > 0` | `list.isEmpty()` / `list.isNotEmpty()` |
| `"" + value` | `"$value"` |
| `Collections.emptyList()` | `emptyList()` |
| `object.equals(other)` | `object == other` |
| `!(x is Foo)` / `!(list.contains(x))` | `x !is Foo` / `x !in list` |
| `for + add` loops | `.map { ... }` |
| Empty catch blocks | At minimum, log the exception |
| Magic numbers/strings | Named constants |
See **[docs/KOTLIN.md](../../../docs/KOTLIN.md)** for the full list of Kotlin idioms, common anti-patterns and their fixes, and scope function guidance.

### KDoc for Public APIs

Document what, why, and gotchas. Link to AWS API docs where relevant.
Document what, why, and gotchas. Link to AWS API docs where relevant. See **[docs/KOTLIN.md](../../../docs/KOTLIN.md)** for KDoc conventions.

## Checklist

Expand Down
8 changes: 2 additions & 6 deletions .claude/skills/review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,8 @@ Evaluate changes against these categories, in priority order:
- Backtick names, `internal class`, AssertJ assertions?

### 4. Kotlin Idioms
- Null safety (`?.`, `?:`) over explicit null checks
- Expression bodies for single-expression functions
- Named `it` in nested/non-trivial lambdas
- `when` over long if-else chains
- `isEmpty()`/`isNotEmpty()` over size checks
- String templates over concatenation

See **[docs/KOTLIN.md](../../../docs/KOTLIN.md)** for the full list of idioms and anti-patterns to check (null safety, expression bodies, named `it`, `when`, `isEmpty()`/`isNotEmpty()`, string templates).

### 5. Documentation & Changelog
- Is `CHANGELOG.md` updated for user-facing changes?
Expand Down
2 changes: 1 addition & 1 deletion .claude/skills/test/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ description: Write, update, or fix tests. Use when asked to test code, create te

# Test Skill — S3Mock

Read **[docs/TESTING.md](../../../docs/TESTING.md)** and `AGENTS.md` (root + relevant module) before writing tests — they define test types, base classes, naming conventions, and running commands.
Read **[docs/TESTING.md](../../../docs/TESTING.md)**, **[docs/KOTLIN.md](../../../docs/KOTLIN.md)**, and `AGENTS.md` (root + relevant module) before writing tests — they define test types, base classes, naming conventions, and running commands.

## Key Conventions (from AGENTS.md)

Expand Down
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ docker/ # Docker image build

## Code Style

**Kotlin idioms**: Data classes for DTOs, null safety, expression bodies, constructor injection
See **[docs/KOTLIN.md](docs/KOTLIN.md)** for Kotlin idioms, naming conventions, common anti-patterns, and KDoc guidelines.

**Spring**: `@RestController`, `@Service`, `@Component`, constructor injection over field injection

Expand Down
72 changes: 72 additions & 0 deletions docs/KOTLIN.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# Kotlin Guidelines — S3Mock

Canonical reference for Kotlin idioms, naming conventions, and code quality standards used across this project.

## Idioms

### Null Safety
- Use `?.`, `?:`, and nullable types instead of explicit null checks
- `x?.foo()` over `if (x != null) { x.foo() }`
- `x ?: throw ...` or `requireNotNull(x)` over `if (x == null) throw ...`

### Immutability
- Prefer `val` over `var`, especially for public API properties

### Expression Bodies
- Use for single-expression functions: `fun foo() = bar()`

### Lambda Parameters
- Always name `it` in nested or non-trivial lambdas to avoid shadowing
- `.map { part -> ... }` instead of `.map { it.name }`
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

The lambda-parameter example doesn’t show an equivalent transformation: .map { it.name } becomes .map { part -> ... }, which changes semantics and may confuse readers. Consider using an equivalent explicit-parameter example (e.g., naming the parameter and still returning the same expression) or provide a nested-lambda example that actually demonstrates it shadowing.

Suggested change
- `.map { part -> ... }` instead of `.map { it.name }`
- `.map { part -> part.name }` instead of `.map { it.name }`

Copilot uses AI. Check for mistakes.

### `when` Expressions
- Prefer `when` over `if-else` chains with 3+ branches

### Scope Functions
- Use `.let`/`.also` when they improve readability, not gratuitously
- Use early returns to flatten deeply nested code
- Extract functions: break up methods longer than ~30 lines

### Collections
- `list.isEmpty()` / `list.isNotEmpty()` over `list.size == 0` / `list.size > 0`

### String Templates
- Use `"$value"` over `"" + value` concatenation

### Kotlin Stdlib
- Prefer Kotlin stdlib / JDK APIs over adding new third-party libraries (no Apache Commons)

## Common Anti-Patterns

| Anti-Pattern | Refactor To |
|---|---|
| `if (x != null) { x.foo() }` | `x?.foo()` |
| `if (x == null) throw ...` | `x ?: throw ...` or `requireNotNull(x)` |
| `list.size == 0` / `list.size > 0` | `list.isEmpty()` / `list.isNotEmpty()` |
| `"" + value` | `"$value"` |
| `Collections.emptyList()` | `emptyList()` |
| `object.equals(other)` | `object == other` |
| `!(x is Foo)` / `!(list.contains(x))` | `x !is Foo` / `x !in list` |
| `for + add` loops | `.map { ... }` |
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

The anti-pattern entry “for + add loops → .map { ... }” is a bit too broad: map only fits 1:1 transforms, while many for+add loops are filtering/flattening/side-effecting (better covered by filter/mapNotNull/flatMap/forEach). Consider tightening the wording (e.g., “for loops that build a new list via add(f(x))”) or expanding the suggested alternatives to avoid misleading guidance in this canonical doc.

Suggested change
| `for + add` loops | `.map { ... }` |
| `for` loops that build a new list via `result.add(f(x))` | Collection ops: `.map` / `.filter` / `.mapNotNull` / `.flatMap` / `.forEach` as appropriate |

Copilot uses AI. Check for mistakes.
| Empty catch blocks | At minimum, log the exception |
| Magic numbers/strings | Named constants |

## Naming Conventions

- **Booleans**: `is-`/`has-`/`should-`/`can-` prefixes
- **Collections**: plural nouns
- **Functions**: verb phrases (`verifyBucketExists`, `resolveVersionId`)
- **Avoid abbreviations**: `bucketMetadata` not `bktMd`

## Test Naming

- **Backtick names**: Use descriptive sentences — `` fun `should create bucket successfully`() ``
- **Legacy names**: Refactor `testSomething` camelCase names to backtick style when touching existing tests
- **Visibility**: Mark test classes as `internal`

## KDoc

- Use `/** */` for public APIs; `//` inline comments for rationale
- Comments explain **why**, never **what** — remove comments that restate the code
- Add comments for edge cases, non-obvious S3 semantics, or workarounds
- Link to AWS API docs or GitHub issues where relevant
2 changes: 2 additions & 0 deletions docs/TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ Access `serviceEndpoint`, `serviceEndpointHttp`, and `serviceEndpointHttps` from

## Conventions

See **[docs/KOTLIN.md](KOTLIN.md)** for Kotlin naming conventions (backtick test names, `internal` visibility, naming patterns).

- **Naming**: Backtick names with descriptive sentences — `` fun `should create bucket successfully`() ``
- **Visibility**: Mark test classes `internal`
- **Pattern**: Arrange-Act-Assert
Expand Down
Loading