-
Notifications
You must be signed in to change notification settings - Fork 289
Regression: Emit notification to check CLI version against client #2404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🔒 Entelligence AI Vulnerability Scanner ✅ No security vulnerabilities found! Your code passed our comprehensive security analysis. |
LGTM 👍 |
🌿 Preview your docs: https://boundary-preview-2256cd3e-1376-4aa0-b200-93aaa6d2bf82.docs.buildwithfern.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 95c2364 in 2 minutes and 45 seconds. Click for details.
- Reviewed
306
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/language_server/src/server/api/notifications/did_open.rs:84
- Draft comment:
Restoring the generator version notification is handled well. Usinggenerator_version.as_ref().ok()
ensures that no notification is sent when the version check fails, which aligns with the intended behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply acknowledges that the code is handling a situation correctly, which is not necessary in a code review context.
2. engine/language_server/src/server/api/notifications/did_save_text_document.rs:98
- Draft comment:
The version-check block combining the runtime, client, and generator versions via chained iterators is clever. Ensure that client_version, when present, always conforms to semver. Also, the extraction of effective_flags appears twice; consider consolidating this to avoid duplicate logic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
The comment provides a suggestion to consolidate duplicate logic, which is a valid code improvement suggestion. However, the part about ensuring client_version conforms to semver is asking the author to ensure something, which violates the rules. The comment should be edited to remove the part about ensuring semver compliance.
3. engine/language_server/src/server/api/notifications/did_save_text_document.rs:83
- Draft comment:
The combined version validation (using runtime version, client version, and generator version) is an effective strategy. Just ensure that all parts (especially client_version) are valid semver strings, as this check depends on successful parsing. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and suggests ensuring that all parts are valid semver strings. It doesn't provide a specific code suggestion or point out a potential issue with the code. It violates the rule against making purely informative comments.
4. engine/language_server/src/server/api/notifications/did_save_text_document.rs:79
- Draft comment:
Typo: "againt" should be "against" in the comment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the typo, typos in comments are very minor issues. The meaning is still clear despite the typo. Comments are meant to focus on substantive code issues that need to be fixed, not minor spelling errors that don't affect functionality. The typo could theoretically reduce code readability and professionalism. Documentation quality is important. While documentation quality matters, this is an extremely minor issue that doesn't meaningfully impact understanding. PR comments should focus on more substantial issues. Delete this comment as it focuses on an insignificant typo rather than a meaningful code issue that needs to be fixed.
Workflow ID: wflow_lC5RZFJwcS2IU7CX
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-6660c071-d3fe-42fa-b2cc-ad54676c1778.docs.buildwithfern.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed e0a91d9 in 1 minute and 34 seconds. Click for details.
- Reviewed
46
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/language_server/src/server/api/notifications/did_open.rs:86
- Draft comment:
Use &locked instead of &*locked for clarity. The dereference is unnecessary if locked implements Deref to the project type. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. engine/language_server/src/server/api/notifications/did_save_text_document.rs:6
- Draft comment:
Renamed import from BamlProject to Project for consistency. Ensure all usages align with the new type. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. engine/language_server/src/server/api/notifications/did_save_text_document.rs:75
- Draft comment:
Pass the locked project directly to send_generator_version instead of extracting a separate project reference. This simplifies the code and ensures type consistency with the updated Project alias. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. engine/language_server/src/server/api/notifications/did_save_text_document.rs:78
- Draft comment:
Typo: In the comment, 'againt' should be corrected to 'against'. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_2ZXRHzfKnHnwXv5C
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
🌿 Preview your docs: https://boundary-preview-842c1020-3209-434b-a749-5544e9be27d9.docs.buildwithfern.com |
Emits version notification even when LSP server & generators do not match, so that the correct version can be downloaded.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
e0a91d9
to
94b5b8f
Compare
🌿 Preview your docs: https://boundary-preview-65f41c02-f17c-4fc8-a2d2-bc7df2ed1a71.docs.buildwithfern.com |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
The notification for the check is restored; now a notification is sent regardless of mismatches between LSP & generators. Will not send a notification if generators don't match, since then we don't have a baseline to compare to.
Speculative downloading doesn't make a lot of sense either: download LSP for previous version -> user changes version to match highest version -> download missed. Since # of generators is small then probability of failure is pretty close to 50%.
Important
Restores CLI version check notification and refactors version checking logic for robustness.
get_common_generator_version()
inmod.rs
to centralize version checking logic.send_generator_version()
indid_save_text_document.rs
to handle version notifications.project_diagnostics()
indiagnostics.rs
to handle generator version mismatches.This description was created by
for e0a91d9. You can customize this summary. It will automatically update as commits are pushed.