-
Notifications
You must be signed in to change notification settings - Fork 202
feat: jetbrains extension, syntax highlighting and release automation only #1983
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 Git ↗︎
|
distribution: temurin | ||
java-version: 23 | ||
|
||
- uses: gradle/actions/setup-gradle@v4 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
Uses Step
with: | ||
distribution: temurin | ||
java-version: 23 | ||
- uses: gradle/actions/setup-gradle@v4 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
Uses Step
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 0cb1a70 in 3 minutes and 7 seconds. Click for details.
- Reviewed
3745
lines of code in48
files - Skipped
18
files when reviewing. - Skipped posting
9
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. jetbrains/build.gradle.kts:42
- Draft comment:
Consider filtering out empty strings from the result of splitting the 'platformBundledPlugins' property value. If the property is empty, it might return a list with an empty string, which could cause issues when passed to bundledPlugins(). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a defensive programming suggestion that could prevent issues if the property is empty or contains consecutive commas. However, we don't have evidence that this is actually causing problems. The code is part of a standard JetBrains plugin setup, and if empty strings were a common issue, it would likely be handled in the bundledPlugins() function itself. The comment points out a theoretically possible edge case, but we don't know if empty strings actually cause issues in the bundledPlugins() function. The same pattern is used for platformPlugins without issues. While the suggestion is technically sound, it falls into the category of speculative comments ("If X, then Y is an issue"). Without evidence that empty strings cause actual problems, this is premature optimization. Delete the comment as it's speculative and suggests handling an edge case without evidence that it's actually problematic.
2. jetbrains/src/main/resources/META-INF/plugin.xml:16
- Draft comment:
There is a potential icon mismatch: the plugin.xml specifies an icon path '/icons/baml-lamb-light-mode.svg' while the BamlIcons class loads '/icons/baml-lamb-purple.svg'. Please ensure the icon file used in the plugin XML matches the one loaded in code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment references a mismatch with BamlIcons class, I don't actually see that class in the diff. Without seeing the BamlIcons class, I can't verify if there truly is a mismatch. The comment is making an assumption about code we can't see. This violates the principle of needing strong evidence to keep a comment. The icon mismatch could be a real issue that causes problems at runtime. Icon paths being wrong can lead to missing icons in the UI. While icon mismatches can be real issues, we need to see both sides of the mismatch to verify the problem. Without seeing the BamlIcons class, this is speculation. Delete the comment because we don't have enough context to verify the claimed mismatch - we can't see the BamlIcons class that's referenced.
3. tools/bump-version.py:59
- Draft comment:
The get_current_version() function reads version information from 'tools/versions/engine.cfg', but for the JetBrains plugin version bump this might be expected to come from 'tools/versions/jetbrains.cfg'. This could lead to inconsistencies when bumping the JetBrains version. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. jetbrains/.github/ISSUE_TEMPLATE/config.yml:8
- Draft comment:
Typographical/grammatical suggestion: Consider revising "Check if you search for a help in plugin development." For example, you might change it to "Check if you're looking for help in plugin development." - 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% This is a new file being added, so the comment is about changed code. The text is user-facing and the current grammar is awkward and incorrect. However, this is a minor UI text change that doesn't affect functionality. According to the rules, we should not comment on pure UI changes - we should assume the author chose the text they wanted. The grammar issue could impact user understanding of the help text. Bad grammar in user-facing text could reflect poorly on the project's professionalism. While valid points, the rules explicitly state not to comment on UI changes. We must trust that the author has chosen their desired text. Delete this comment as it violates the rule about not commenting on UI/text changes.
5. jetbrains/CHANGELOG.md:106
- Draft comment:
Typographical error: The changelog entry reads "Replaceorg.jetbrains:annotations
library with ancom.example:exampleLibrary
placeholder." Consider changing "ancom.example:exampleLibrary
" to "acom.example:exampleLibrary
" since the acronym begins with a consonant sound. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While technically correct, this is an extremely minor grammatical issue in a changelog entry. The meaning is perfectly clear either way. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category - it's a trivial change that doesn't impact understanding or functionality. The grammar rule about "a" vs "an" is a legitimate English language rule, and maintaining professional documentation standards could be seen as important. However, this level of nitpicking on grammar in internal documentation like changelogs, when the meaning is completely clear, creates unnecessary noise in code reviews that should focus on more substantive issues. This comment should be deleted as it violates the rule about not making obvious or unimportant comments. The grammatical issue is too minor to warrant a PR comment.
6. jetbrains/README.md:69
- Draft comment:
There appears to be an extra space in the phrase "filter your GitHub repository" (two spaces between "GitHub" and "repository"). - 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 double space, this is an extremely minor formatting issue in a documentation file. It doesn't affect functionality, readability is barely impacted, and it's the kind of nitpicky comment that can distract from more important review feedback. The rules specifically say not to make comments that are obvious or unimportant. The extra space could be considered a documentation quality issue, and some might argue that maintaining consistent spacing is important for professionalism. While consistency is good, this level of nitpicking on spacing in documentation doesn't provide enough value to justify the cognitive overhead it adds to the PR review process. This comment should be deleted as it's too minor and doesn't add meaningful value to the review.
7. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlProjectActivity.kt:11
- Draft comment:
Typo: Consider using 'JetBrains' (with a capital B) instead of 'Jetbrains' in the log message for correct branding consistency. - 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 technically correct about JetBrains branding, this is an internal log message that users won't see. It's a very minor detail that doesn't affect functionality. According to the rules, we shouldn't make comments that are obvious or unimportant, and this feels quite unimportant. The official company name is indeed "JetBrains" and maintaining consistent branding could be considered a quality issue. Maybe this consistency is more important than I initially thought? While branding consistency is good, this is just a debug log message that only developers would see. The rules specifically say not to make comments that are obvious or unimportant. Delete this comment as it's too minor of an issue to warrant a PR comment, especially for an internal log message.
8. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlProjectService.kt:13
- Draft comment:
Typographical issue: Consider using "JetBrains" (with a capital B) instead of "Jetbrains" in the log message for consistency and correct branding. - 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 technically correct about JetBrains branding, this is an extremely minor issue that only affects a debug log message. It doesn't impact functionality, user experience, or code quality. Such nitpicky comments about log messages can create unnecessary noise in code reviews. The branding consistency could be important for professional appearance in logs, especially if logs are shared with JetBrains team or users. While proper branding is good practice, this is an internal debug log message that most users will never see, making it too minor to warrant a code review comment. Delete this comment as it's too minor and doesn't affect functionality or code quality in any meaningful way.
9. jetbrains/src/test/testData/rename/foo.xml:2
- Draft comment:
Typographical issue: It looks like there's an unintended<caret>
inserted in the tag<a<caret>1>
. This should likely be corrected, e.g.,<a1>
. - 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% Given this is in a test/testData directory and specifically in a 'rename' folder, the<caret>
marker is almost certainly intentional. Such markers are commonly used in IDE test cases to indicate cursor position for testing rename refactoring functionality. The comment misunderstands the purpose of this test file. Could there be a case where this really is a typo and the test file is incorrect? Maybe the caret syntax is different in this codebase? The location in a test data directory and the specific 'rename' folder context strongly suggests this is intentional test data. The format matches common IDE test markers. The comment should be deleted as it misunderstands the purpose of a test file marker that is intentionally placed.
Workflow ID: wflow_JUKIgr36DLwGP6nC
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
fun testProjectService() { | ||
val projectService = project.service<BamlProjectService>() | ||
|
||
assertNotSame(projectService.getRandomNumber(), projectService.getRandomNumber()) |
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.
Using assertNotSame
to compare two random numbers may not reliably detect differences because of integer caching. Consider using assertNotEquals
to compare the numeric values instead.
assertNotSame(projectService.getRandomNumber(), projectService.getRandomNumber()) | |
assertNotEquals(projectService.getRandomNumber(), projectService.getRandomNumber()) |
|---------------------------|--------------------------------------------------------------------------------------------------------------| | ||
| `PRIVATE_KEY` | Certificate private key, should contain: `-----BEGIN RSA PRIVATE KEY----- ... -----END RSA PRIVATE KEY-----` | | ||
| `PRIVATE_KEY_PASSWORD` | Password used for encrypting the certificate file. | | ||
| `CERTIFICATE_CHAIN` | Certificate chain, should contain: `-----BEGIN CERTIFICATE----- ... -----END CERTIFICATE----` | |
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.
Typo in the certificate chain example: it shows -----END CERTIFICATE----
(only 4 dashes at the end) but it should be -----END CERTIFICATE-----
(with 5 dashes).
| `CERTIFICATE_CHAIN` | Certificate chain, should contain: `-----BEGIN CERTIFICATE----- ... -----END CERTIFICATE----` | | |
| `CERTIFICATE_CHAIN` | Certificate chain, should contain: `-----BEGIN CERTIFICATE----- ... -----END CERTIFICATE-----` | |
current_version = 0.88.0 | ||
commit = False | ||
tag = False | ||
parse = ^(?P<major>\d+)\.(?P<minor>\d+).(?P<patch>\d+)$ |
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.
The regex in the parse
line uses an escaped dot for the first separator (\.
) but the second separator is just a dot. In order to match a literal dot, it should be escaped as well (\.
). Please update the pattern to: ^(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)$
.
parse = ^(?P<major>\d+)\.(?P<minor>\d+).(?P<patch>\d+)$ | |
parse = ^(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)$ |
Implement jetbrains extension and set up jetbrains release automation.
The only thing implemented in the jetbrains extension right now is syntax highlighting. We still need to:
But this lets us start iterating on it asap in prod.
Important
Adds JetBrains extension with syntax highlighting and sets up release automation.
BamlFileType
,BamlLanguage
, andBamlTextMateBundleProvider
.BamlToolWindowFactory
for a tool window with a basic HTML content placeholder.BamlProjectActivity
andBamlProjectService
for project-level activities and services.build-jetbrains-release.reusable.yaml
for building and testing the JetBrains plugin.release.yml
to include JetBrains release steps.publish-to-jetbrains-marketplace
job inrelease.yml
.gradle.properties
,build.gradle.kts
, andsettings.gradle.kts
for project configuration..github/ISSUE_TEMPLATE
for bug reports and feature requests.dependabot.yml
for dependency updates.CHANGELOG.md
,LICENSE
, andREADME.md
for documentation..gitignore
and.run
configurations for IntelliJ IDEA.This description was created by
for 0cb1a70. You can customize this summary. It will automatically update as commits are pushed.