-
Notifications
You must be signed in to change notification settings - Fork 381
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
CARGO: Annotate basic docs in cargo.toml #1793
Conversation
@@ -20,9 +20,10 @@ | |||
<!-- please see https://confluence.jetbrains.com/display/IDEADEV/Plugin+Compatibility+with+IntelliJ+Platform+Products | |||
on how to target different products --> | |||
|
|||
<depends>com.intellij.modules.lang</depends> |
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.
Hm, I believe this is a required dependency?
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.
I have no idea
<depends optional="true" config-file="idea-only.xml">com.intellij.modules.java</depends> | ||
<depends optional="true" config-file="clion-only.xml">com.intellij.modules.clion</depends> | ||
<depends optional="true" config-file="clion-only.xml">com.intellij.modules.clion</depends> |
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.
Duplicated line?
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.
jo
Not sure what's up with tests, restarted the build. |
build.gradle.kts
Outdated
@@ -13,6 +13,7 @@ import org.gradle.api.tasks.testing.logging.TestExceptionFormat | |||
buildscript { | |||
repositories { | |||
maven { setUrl("https://jitpack.io") } | |||
maven { setUrl("http://dl.bintray.com/jetbrains/intellij-plugin-service") } |
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.
Is it possible to use https for this repository?
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.
fixed it
4460377
to
5bddfe0
Compare
local error if i run the test
|
build.gradle.kts
Outdated
intellij { pluginName = "intellij-rust" } | ||
intellij { | ||
pluginName = "intellij-rust" | ||
setPlugins("org.toml.lang:0.0.1.1759") |
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.
I think we should specify a path to Gradle subproject here, like project(':toml')
.
5bddfe0
to
e939fc6
Compare
Changed, but this i didn't fix my problem. |
@farodin91 looking into this, and I don't know what's going on here :( I think I had a similar (closed source) project with exactly this setup, and it did work there... |
@matklad Any suggestions? |
@farodin91 I think I've found a bug in intellij gradle plugin, will try to fix it soon. |
The workaround to change
|
This breaks all for me. |
All Toml types are unresolved |
Found I copied with look into the code. ;) |
e939fc6
to
c020efc
Compare
Resolved |
The fix for the gradle plugin: JetBrains/intellij-platform-gradle-plugin#238 |
@matklad merge conflicts fixed |
d80c5b0
to
97639b0
Compare
@matklad Merge conflicts are fixed. |
Looking good @farodin91 The gradle plugin is not released yet, but I think we can use snapshot versions for some time. One thing I would like to do here though is to change |
97639b0
to
bc6eb3d
Compare
@matklad fixed |
bc6eb3d
to
acb4d00
Compare
@maklad resolved latest merge conflict |
@farodin91 What about making dependency on the TOML plugin optional? #1793 (comment) |
I rebased the false commit. |
acb4d00
to
36f2494
Compare
@matklad should I move this to linemarkers |
@farodin91 I think this should all be moved to a top-level I am also still a bit uneasy about depending on the TOML plugin API, as they are not great at the moment, and changing them later will result in class cast exceptions. I'll try to make TOML APIs more stable. |
Branch with a (hopefully :) ) better TOML plugin: https://github.com/intellij-rust/intellij-rust/tree/toml |
@matklad Will wait until your PR is merged. |
@matklad I try to fix all problems. |
@matklad Inlinetable is longer a TomlValue |
36f2494
to
e361c12
Compare
The toml plugin is released and the dependency is updated. |
@matklad You changed the build.gradle.kts but intellij is trying to use toml in versio 0.2.0.8 not 0.2.0.9 |
Probably some issues with caches. Deleting |
e361c12
to
51e60dd
Compare
@matklad Do you like to review? |
class CargoCrateDocLineMarkerProvider : LineMarkerProvider { | ||
override fun getLineMarkerInfo(element: PsiElement): LineMarkerInfo<PsiElement>? = null | ||
|
||
override fun collectSlowLineMarkers(elements: MutableList<PsiElement>, result: MutableCollection<LineMarkerInfo<PsiElement>>) { |
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.
Let's add an ABI check here, the same way it is done in CargoTomlCompletionContributor?
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.
I added the same. Is this okay?
fun `test no link to doc`() = doTestByText(""" | ||
[dependencies] | ||
hello_utils = { path = "hello_utils" } | ||
""") |
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.
👍 great tests!
2a34892
to
ba2ccba
Compare
@matklad I have no idea. Why nightly crashes? |
@farodin91 I think it's
See how other lines markers were fixed recently. |
ba2ccba
to
3951110
Compare
@matklad Resolved ? Do you like to review? |
|
||
private fun genLineMarkerInfo(element: PsiElement, name: String, version: String?): LineMarkerInfo<PsiElement>? { | ||
val version = version ?: return null | ||
val anchor = PsiTreeUtil.getDeepestFirst(element) |
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.
Hm, it's super non-obvious what element is returned here... Could we be somehow more specific?
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.
I change this to fix the problem with the leaf.
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.
Is this okay?
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.
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.
better?
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.
Most definitely!
3951110
to
8a28e8b
Compare
8a28e8b
to
5c4dc21
Compare
@bors r+ Thanks! |
bors r=matklad |
Fixes #1776
@matklad Any idea to fix the problem with the test?