Repository navigation
refactor: adapt eventide APIs and deco options - #363
Closed
16bit-ykiko wants to merge 3 commits into
Closed
16bit-ykiko wants to merge 3 commits into
16bit-ykiko wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR migrates feature and test code from language-level protocol types to LSP/IPC protocol types, adds deco-based CLI parsing and options handling, updates CMake FetchContent and target links (removing tomlplusplus and changing eventide feature flags), and adds explicit returns and small toolchain/formatting adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
16bit-ykiko
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/feature/inlay_hint_tests.cpp (1)
27-29:⚠️ Potential issue | 🟠 MajorInconsistent namespace usage:
eventide::language::PositionMappershould beeventide::ipc::lsp::PositionMapper.Line 28 still uses
eventide::language::PositionMapper, but the rest of the codebase has migrated toeventide::ipc::lsp::PositionMapper(as shown insrc/feature/feature.hlines 23-24). This inconsistency may cause compilation issues or link against the wrong implementation.🐛 Suggested fix
hints_map.clear(); - eventide::language::PositionMapper converter(tester.unit->interested_content(), - feature::PositionEncoding::UTF8); + feature::PositionMapper converter(tester.unit->interested_content(), + feature::PositionEncoding::UTF8);Or use the fully qualified name:
hints_map.clear(); - eventide::language::PositionMapper converter(tester.unit->interested_content(), - feature::PositionEncoding::UTF8); + eventide::ipc::lsp::PositionMapper converter(tester.unit->interested_content(), + feature::PositionEncoding::UTF8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/feature/inlay_hint_tests.cpp` around lines 27 - 29, Update the inconsistent type usage by replacing eventide::language::PositionMapper with eventide::ipc::lsp::PositionMapper where it is constructed (the line creating PositionMapper with tester.unit->interested_content() and feature::PositionEncoding::UTF8); ensure any related includes or using directives reference the ipc::lsp namespace so the test links against the correct implementation and compiles consistently with the rest of the codebase.tests/unit/feature/document_link_tests.cpp (1)
26-30:⚠️ Potential issue | 🟠 MajorInconsistent namespace usage:
eventide::language::PositionMappershould be migrated.Similar to the issue in
tests/unit/feature/inlay_hint_tests.cpp, line 27-28 useseventide::language::PositionMapperwhile the rest of the codebase has migrated toeventide::ipc::lsp::PositionMapper.🐛 Suggested fix
auto to_local_range(const protocol::Range& range) -> LocalSourceRange { - eventide::language::PositionMapper converter(tester.unit->interested_content(), - feature::PositionEncoding::UTF8); + feature::PositionMapper converter(tester.unit->interested_content(), + feature::PositionEncoding::UTF8); return LocalSourceRange(converter.to_offset(range.start), converter.to_offset(range.end)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/feature/document_link_tests.cpp` around lines 26 - 30, The helper function to_local_range currently constructs a PositionMapper using the old namespace eventide::language::PositionMapper; change that to use the migrated eventide::ipc::lsp::PositionMapper so the codebase is consistent: update the constructor call inside to_local_range to instantiate eventide::ipc::lsp::PositionMapper(tester.unit->interested_content(), feature::PositionEncoding::UTF8) and return LocalSourceRange(converter.to_offset(range.start), converter.to_offset(range.end)) as before.
🧹 Nitpick comments (1)
cmake/package.cmake (1)
44-45: Usingmainbranch for eventide may cause build instability.Pinning to a specific Git tag or commit SHA is recommended for reproducible builds. The
mainbranch can change at any time, potentially breaking the build or introducing unexpected behavior.♻️ Suggested fix: Pin to a specific commit or tag
FetchContent_Declare( eventide GIT_REPOSITORY https://github.com/clice-io/eventide - GIT_TAG main + GIT_TAG <specific-commit-sha-or-tag> GIT_SHALLOW TRUE )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/package.cmake` around lines 44 - 45, The package.cmake entry currently pins the eventide dependency to the mutable branch via the GIT_TAG value "main"; update this to a stable, immutable identifier by replacing GIT_TAG main with a specific release tag or commit SHA for eventide (and keep GIT_SHALLOW TRUE if desired) so builds are reproducible—locate the GIT_TAG setting in cmake/package.cmake and set it to the chosen tag or commit SHA instead of "main".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clice.cc`:
- Around line 12-15: The declaration for the CLI flag `mode` (the DecoKV block
around the variable `mode`) is inconsistent with the runtime check that treats
it as required; update the DecoKV for `mode` to set required = true so parsing
fails early, and then remove the redundant runtime presence check that inspects
`mode` (the block around the current lines 66-68), or alternatively keep the
runtime check but change its error text to match the parser's style—prefer the
first option: set required = true in the DecoKV for `mode` and delete the manual
validation that exits when `mode` is empty.
In `@tests/unit/unit_tests.cc`:
- Around line 23-29: The code silently ignores parse errors from
deco::cli::parse<TestOptions> and proceeds with an empty filter; instead, check
parsed.has_value() and fail fast when it is false: retrieve and log the parse
error (e.g., from parsed.error() or equivalent) and exit with a non-zero status
(or assert/FAIL in tests) before using parsed->options.test_filter; update the
block around the parsed variable in unit_tests.cc to mirror the main
entrypoint's behavior by emitting the error message and terminating when parsing
fails.
---
Outside diff comments:
In `@tests/unit/feature/document_link_tests.cpp`:
- Around line 26-30: The helper function to_local_range currently constructs a
PositionMapper using the old namespace eventide::language::PositionMapper;
change that to use the migrated eventide::ipc::lsp::PositionMapper so the
codebase is consistent: update the constructor call inside to_local_range to
instantiate
eventide::ipc::lsp::PositionMapper(tester.unit->interested_content(),
feature::PositionEncoding::UTF8) and return
LocalSourceRange(converter.to_offset(range.start),
converter.to_offset(range.end)) as before.
In `@tests/unit/feature/inlay_hint_tests.cpp`:
- Around line 27-29: Update the inconsistent type usage by replacing
eventide::language::PositionMapper with eventide::ipc::lsp::PositionMapper where
it is constructed (the line creating PositionMapper with
tester.unit->interested_content() and feature::PositionEncoding::UTF8); ensure
any related includes or using directives reference the ipc::lsp namespace so the
test links against the correct implementation and compiles consistently with the
rest of the codebase.
---
Nitpick comments:
In `@cmake/package.cmake`:
- Around line 44-45: The package.cmake entry currently pins the eventide
dependency to the mutable branch via the GIT_TAG value "main"; update this to a
stable, immutable identifier by replacing GIT_TAG main with a specific release
tag or commit SHA for eventide (and keep GIT_SHALLOW TRUE if desired) so builds
are reproducible—locate the GIT_TAG setting in cmake/package.cmake and set it to
the chosen tag or commit SHA instead of "main".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7372c28-9655-46ef-a743-efc13665c596
📒 Files selected for processing (28)
.gitignoreCMakeLists.txtcmake/package.cmakesrc/clice.ccsrc/compile/diagnostic.cppsrc/compile/toolchain.cppsrc/feature/code_completion.cppsrc/feature/diagnostics.cppsrc/feature/document_links.cppsrc/feature/document_symbols.cppsrc/feature/feature.hsrc/feature/folding_ranges.cppsrc/feature/formatting.cppsrc/feature/hover.cppsrc/feature/inlay_hints.cppsrc/feature/semantic_tokens.cppsrc/feature/signature_help.cppsrc/semantic/resolver.cpptests/unit/feature/code_completion_tests.cpptests/unit/feature/document_link_tests.cpptests/unit/feature/document_symbol_tests.cpptests/unit/feature/folding_range_tests.cpptests/unit/feature/hover_tests.cpptests/unit/feature/inlay_hint_tests.cpptests/unit/feature/semantic_tokens_tests.cpptests/unit/feature/signature_help_tests.cpptests/unit/test/tester.htests/unit/unit_tests.cc
💤 Files with no reviewable changes (8)
- src/feature/semantic_tokens.cpp
- src/feature/inlay_hints.cpp
- src/feature/signature_help.cpp
- src/feature/hover.cpp
- src/feature/formatting.cpp
- src/feature/document_symbols.cpp
- src/feature/code_completion.cpp
- src/feature/folding_ranges.cpp
Comment on lines
+12
to
+15
| DecoKV(names = {"--mode"}; | ||
| help = "Running mode: pipe, socket, stateless-worker, stateful-worker"; | ||
| required = false;) | ||
| <std::string> mode; |
There was a problem hiding this comment.
Inconsistency between declaration and runtime validation for --mode.
The --mode option is declared with required = false on line 14, but line 66-68 treats it as required at runtime. Consider either:
- Setting
required = truein the declaration so parsing fails early with a proper message, or - Keeping the current approach but ensuring the error message matches what the library would produce.
💡 Suggested fix: Mark mode as required in declaration
DecoKV(names = {"--mode"};
help = "Running mode: pipe, socket, stateless-worker, stateful-worker";
- required = false;)
+ required = true;)
<std::string> mode;Then remove the redundant check:
- if(!opts.mode.has_value()) {
- std::println(stderr, "error: --mode is required");
- return 1;
- }
-
- std::println("mode: {}", *opts.mode);
+ std::println("mode: {}", *opts.mode);Also applies to: 66-68
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 12-12: syntax error
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/clice.cc` around lines 12 - 15, The declaration for the CLI flag `mode`
(the DecoKV block around the variable `mode`) is inconsistent with the runtime
check that treats it as required; update the DecoKV for `mode` to set required =
true so parsing fails early, and then remove the redundant runtime presence
check that inspects `mode` (the block around the current lines 66-68), or
alternatively keep the runtime check but change its error text to match the
parser's style—prefer the first option: set required = true in the DecoKV for
`mode` and delete the manual validation that exits when `mode` is empty.
Comment on lines
+23
to
+29
| auto args = deco::util::argvify(argc, argv); | ||
| auto parsed = deco::cli::parse<TestOptions>(args); | ||
|
|
||
| std::string_view filter = {}; | ||
| if(parsed.has_value() && parsed->options.test_filter.has_value()) { | ||
| filter = *parsed->options.test_filter; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '44,60p' src/clice.cc
printf '\n---\n'
sed -n '23,31p' tests/unit/unit_tests.ccRepository: clice-io/clice
Length of output: 875
🏁 Script executed:
# Verify the parse error handling pattern across the codebase
rg "deco::cli::parse" -A 5 -B 1Repository: clice-io/clice
Length of output: 819
Fail fast when Deco rejects the CLI.
If deco::cli::parse<TestOptions> returns an error, the code silently continues with an empty filter and runs the full test suite. This differs from the main entrypoint (src/clice.cc), which explicitly checks !result.has_value() and exits with an error message. Invalid or typo'd --test-filter invocations will silently succeed instead of failing.
Suggested fix
auto args = deco::util::argvify(argc, argv);
auto parsed = deco::cli::parse<TestOptions>(args);
+ if(!parsed.has_value()) {
+ return 1;
+ }
std::string_view filter = {};
- if(parsed.has_value() && parsed->options.test_filter.has_value()) {
+ if(parsed->options.test_filter.has_value()) {
filter = *parsed->options.test_filter;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| auto args = deco::util::argvify(argc, argv); | |
| auto parsed = deco::cli::parse<TestOptions>(args); | |
| std::string_view filter = {}; | |
| if(parsed.has_value() && parsed->options.test_filter.has_value()) { | |
| filter = *parsed->options.test_filter; | |
| } | |
| auto args = deco::util::argvify(argc, argv); | |
| auto parsed = deco::cli::parse<TestOptions>(args); | |
| if(!parsed.has_value()) { | |
| return 1; | |
| } | |
| std::string_view filter = {}; | |
| if(parsed->options.test_filter.has_value()) { | |
| filter = *parsed->options.test_filter; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/unit_tests.cc` around lines 23 - 29, The code silently ignores
parse errors from deco::cli::parse<TestOptions> and proceeds with an empty
filter; instead, check parsed.has_value() and fail fast when it is false:
retrieve and log the parse error (e.g., from parsed.error() or equivalent) and
exit with a non-zero status (or assert/FAIL in tests) before using
parsed->options.test_filter; update the block around the parsed variable in
unit_tests.cc to mirror the main entrypoint's behavior by emitting the error
message and terminating when parsing fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores