-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Delegate opening files for Rider to the RiderPathLocator NuGet package #79958
Delegate opening files for Rider to the RiderPathLocator NuGet package #79958
Conversation
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
Show resolved
Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
Outdated
Show resolved
Hide resolved
aff533c
to
1d4616b
Compare
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.
LGTM with two caveats:
- I don't actually have Rider so I didn't test this, but the +1 for the line got removed. Does this open the correct line (watch out for C#: Fix line in OpenInExternalEditor #79404 when testing this)
- The name of this PR/commit is kinda non-descriptive and I think something along the lines of
Delegate opening files in Rider to the Rider locator NuGet package
would be 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.
Only some minor nitpicks, LGTM otherwise.
Also, don't forget to update the commit message, it's more important than the PR title since it's what shows up in the git history.
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
Outdated
Show resolved
Hide resolved
modules/mono/editor/GodotTools/GodotTools/Ides/Rider/RiderPathManager.cs
Outdated
Show resolved
Hide resolved
1d4616b
to
369597e
Compare
I assume they moved that to the NuGet package, but would be nice to confirm. By the way, @van800 the NuGet package points to https://github.com/JetBrains/resharper-unity/ but it doesn't seem like that repository has the source code for the package (or maybe I'm looking at the wrong branch). |
It will get synchronized soon from the internal repo. I will post another message here, once that is done. |
Does this still work after #79404 has been merged? @RedworkDE was correct that now the code no longer adds 1 to the line number and So make sure it opens the file at the correct line number. Also, can you rename the Rider option to include Fleet? Since it seems to support both. Something like this: diff --git a/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs b/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs
index 622a155d37..8bda2706a5 100644
--- a/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs
+++ b/modules/mono/editor/GodotTools/GodotTools/GodotSharpEditor.cs
@@ -538,7 +538,7 @@ namespace GodotTools
settingsHintStr += $",Visual Studio:{(int)ExternalEditorId.VisualStudio}" +
$",MonoDevelop:{(int)ExternalEditorId.MonoDevelop}" +
$",Visual Studio Code:{(int)ExternalEditorId.VsCode}" +
- $",JetBrains Rider:{(int)ExternalEditorId.Rider}" +
+ $",JetBrains Rider and Fleet:{(int)ExternalEditorId.Rider}" +
$",Custom:{(int)ExternalEditorId.CustomEditor}";
}
else if (OS.IsMacOS)
@@ -546,14 +546,14 @@ namespace GodotTools
settingsHintStr += $",Visual Studio:{(int)ExternalEditorId.VisualStudioForMac}" +
$",MonoDevelop:{(int)ExternalEditorId.MonoDevelop}" +
$",Visual Studio Code:{(int)ExternalEditorId.VsCode}" +
- $",JetBrains Rider:{(int)ExternalEditorId.Rider}" +
+ $",JetBrains Rider and Fleet:{(int)ExternalEditorId.Rider}" +
$",Custom:{(int)ExternalEditorId.CustomEditor}";
}
else if (OS.IsUnixLike)
{
settingsHintStr += $",MonoDevelop:{(int)ExternalEditorId.MonoDevelop}" +
$",Visual Studio Code:{(int)ExternalEditorId.VsCode}" +
- $",JetBrains Rider:{(int)ExternalEditorId.Rider}" +
+ $",JetBrains Rider and Fleet:{(int)ExternalEditorId.Rider}" +
$",Custom:{(int)ExternalEditorId.CustomEditor}";
}
|
369597e
to
8654fe8
Compare
8654fe8
to
7f8e3ab
Compare
Thank you, fixed both. |
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.
LGTM, but I haven't tested since I don't have Rider.
Thanks! |
OpenFile (for Rider and Fleet) functionality is now managed by the JetBrains nuget.
When both Rider and Fleet are installed, Rider would be more preferable, which would fix #78832 (comment)