Skip to content

Extract shared is_valid_class_name to beamtalk-core#2820

Merged
jamesc merged 3 commits into
mainfrom
refactor/extract-is-valid-class-name
Jul 4, 2026
Merged

Extract shared is_valid_class_name to beamtalk-core#2820
jamesc merged 3 commits into
mainfrom
refactor/extract-is-valid-class-name

Conversation

@jamesc

@jamesc jamesc commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Summary

validate_class_name was duplicated between beamtalk-lsp and beamtalk-mcp with slightly diverging implementations. This PR extracts the canonical boolean check to beamtalk_core::source_analysis::is_valid_class_name so the naming rule lives once in the correct DDD bounded context (Source Analysis → Language Service).

Changes

  • crates/beamtalk-core/src/source_analysis/mod.rs: Add pub fn is_valid_class_name(name: &str) -> bool — non-empty, starts with ASCII uppercase, all chars alphanumeric or _. Includes 5 unit tests.
  • crates/beamtalk-lsp/src/server.rs: validate_class_name delegates the structural check to is_valid_class_name; keeps its detailed per-character error messages for the LSP Result<(), String> type.
  • crates/beamtalk-mcp/src/server.rs: validate_class_name delegates directly to is_valid_class_name; keeps its rmcp::ErrorData error type.

No behaviour change — both implementations agreed on which names are valid. No new crate dependencies (both LSP and MCP already depend on beamtalk-core).


Generated by Claude Code

validate_class_name was duplicated between beamtalk-lsp and beamtalk-mcp
with slightly diverging logic. Extract the canonical boolean check to
beamtalk_core::source_analysis::is_valid_class_name so the rule lives
once in the right bounded context. Both crates keep their own error types
and messages; they delegate the structural check to the shared function.
Comment thread crates/beamtalk-lsp/src/server.rs
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

✅ Claude Review — PASS

Review Summary

Overall: PASS — clean refactoring, no issues.

This PR extracts the class-name validation predicate into beamtalk-core and delegates to it from both the LSP and MCP servers. The incremental commit replaces .unwrap() with a documented-invariant .expect(...).

Correctness

The invariant claimed by the .expect() message in crates/beamtalk-lsp/src/server.rs is airtight:

  • is_valid_class_name returns false → at least one of: (empty, not uppercase-start, has-bad-char)
  • validate_class_name checks empty → returns Err if empty
  • validate_class_name checks uppercase-start → returns Err if missing
  • The only remaining reason is_valid_class_name could have returned false is a bad character, so chars().find(|c| !(alphanumeric || '_')) must return Some

No panic is reachable at runtime. Existing LSP tests (validate_class_name_rejects_bad_shapes at line 4957) cover this path with "Bad!" — starts uppercase, contains ! — exercising the .expect() branch.

The MCP simplification is equivalent to the previous three-condition check. Behavior is preserved across both surfaces.

Blockers

None.

Suggestions

None.

Nits

None.

After the three exhaustive checks (empty / not-uppercase / bad-char),
the terminal Err could never fire. Replace the if-let with an unwrap()
that is guaranteed to succeed at that point, eliminating the dead branch.
Comment thread crates/beamtalk-lsp/src/server.rs Outdated
Addresses review nit: the bad-char search is safe today but silently
forward-couples on is_valid_class_name's exact conditions. Use .expect()
to name the invariant so a future added condition surfaces a clear panic
message rather than a bare unwrap on a should-be-validation path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015RFHaym8aCseDmmZ417jn2
@jamesc jamesc merged commit ea4a60f into main Jul 4, 2026
9 checks passed
@jamesc jamesc deleted the refactor/extract-is-valid-class-name branch July 4, 2026 08:59
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.

2 participants