fixed

[navicore]

No description provided.

[claude]

Code Review

Overview

This PR makes two independent improvements:

1. LSP completion kind: Changes CompletionItemKind::FUNCTION to CompletionItemKind::OPERATOR for all word completions to prevent editors from auto-inserting () after confirmation.
2. String builtin docs: Adds stack-effect notation (e.g. ( str substring -- Int )) to several string builtin doc strings and standardises punctuation.

---

Positive observations

• The OPERATOR kind change is well-reasoned. For a concatenative (stack-based) language like Seq, having editors auto-insert () on completion is genuinely harmful. The comment in make_word_completion explains why clearly, and the back-references from the other two sites keep the file DRY.
• Adding stack-effect signatures to the doc strings is an improvement — they give users a compact, conventional way to understand operand order.

---

Issues & suggestions

1. Inconsistent doc coverage (text.rs)

Stack-effect notation was added to 4 string builtins but not the remaining two in the same block:

docs.insert("string.empty?", "Check if string is empty.");
docs.insert("string.equal?", "Check if two strings are equal.");

Expected additions:

• string.empty? → ( str -- Bool )
• string.equal? → ( str str -- Bool )

If you're establishing a notation convention, it should be applied uniformly in the same commit — partial adoption makes the docs look inconsistent to users.

2. Punctuation inconsistency in doc strings

The updated entries use a semicolon to join two clauses ("Find substring; returns index…") while the original unchanged entries continue using two sentences separated by a period. Consider picking one style and applying it throughout.

3. PR title

"fixed" is not informative. A title like "lsp: use OPERATOR completion kind for concatenative words; add stack effects to string docs" would make git log and changelogs useful.

4. No tests for the OPERATOR kind change

There are no new or updated tests verifying that completion items carry OPERATOR instead of FUNCTION. Even a lightweight assertion would prevent future regressions:

assert_eq!(item.kind, Some(CompletionItemKind::OPERATOR));

5. Minor: OPERATOR vs. alternative kinds

OPERATOR is semantically reasonable but conventionally associated with symbols (+, -, *). Named words like string.find sit closer to KEYWORD in some clients' mental model. Worth checking how nvim-cmp and VS Code actually render OPERATOR for alphabetic labels (icon, font weight) to confirm it's the best fit.

---

Summary

Area	Status
LSP kind change (correctness)	✅ Good fix, well-commented
Stack-effect notation	⚠️ Incomplete — string.empty? and string.equal? skipped
Punctuation / style consistency	⚠️ Mixed styles across entries
PR title	⚠️ Too vague
Test coverage	⚠️ No tests for the kind change

The core logic is correct. Addressing the doc inconsistency and adding at least a minimal test for the completion kind would make this mergeable with confidence.

[claude]

Code Review (updated)

Overview

This PR makes two well-scoped improvements:

1. LSP completion kind — changes CompletionItemKind::FUNCTION → CompletionItemKind::OPERATOR at all three completion sites so editors (nvim-cmp, VS Code) stop auto-inserting () after confirmed words.
2. String builtin docs — adds stack-effect notation uniformly to all string builtins and tidies punctuation.

---

What's improved since the last review

• Stack-effect signatures are now applied uniformly — string.empty? and string.equal? are no longer missing.
• A regression test (test_word_completion_kind_is_operator) was added to guard the OPERATOR kind change for built-ins.

---

Remaining issues

1. Test coverage gap — third completion path untested

The test calls get_builtin_completions(), but the PR changes completion kind in three places. The third site (around line 349 in completion.rs) is a distinct code path that constructs snippet-style items. It is not covered by the new test. A reader can verify it has the same OPERATOR annotation, but a future refactor could regress it silently.

Suggestion: extract a small helper that asserts every non-KEYWORD CompletionItem in a Vec<CompletionItem> uses OPERATOR, then call it from two separate tests — one for get_builtin_completions(), one for the snippet path.

2. Type-name capitalisation inconsistency in doc strings

The stack-effect signatures use mixed capitalisation:

Builtin	Type style
string.find	Int (capital)
string.contains	Bool (capital)
string.split	list (lowercase)
string.join	string (lowercase)

Pick one convention. Since Int and Bool are already capitalised, List and String would be consistent.

3. PR title

"fixed" is still not informative. Something like "lsp: use OPERATOR completion kind; add stack effects to string docs" would make git log and changelog generation useful.

---

Minor / optional observations

• OPERATOR is semantically reasonable for a concatenative language, but alphabetic labels like string.find may render with an operator icon (⊕, ±) in some clients, which looks odd. Worth a quick visual check in both nvim-cmp and VS Code before merging, just to confirm the UX is acceptable.
• The comment in make_word_completion is the most detailed explanation of the rationale. The back-references at the other two sites (// OPERATOR — see make_word_completion for rationale.) keep things DRY — good pattern.

---

Summary

Area	Status
OPERATOR kind change (correctness)	✅ Correct and well-commented
Stack-effect notation coverage	✅ Now complete
Regression test for built-ins	✅ Added
Third completion path test	⚠️ No test coverage
Type-name capitalisation in docs	⚠️ Mixed Int/Bool vs list/string
PR title	⚠️ Still too vague

The core fix is correct and the comment quality is good. Addressing the capitalisation inconsistency and third-path test gap would make this solid.