godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 96 forks source link

Release a new version of GodotTools.IdeMessaging nuget? #9502

Closed van800 closed 6 months ago

van800 commented 6 months ago

Describe the project you are working on

I am developer of the Godot support plugin for Rider.

Describe the problem or limitation you are having in your project

The path to the metadata in the current source is Path.Combine(godotProjectDir, ".godot", "mono", "metadata"); https://github.com/godotengine/godot/blob/a7b860250f305f6cbaf61c30f232ff3bbdfdda0b/modules/mono/editor/GodotTools/GodotTools.IdeMessaging/Client.cs#L126

But in the latest available (1.1.1) GodotTools.IdeMessaging nuget it is old one: string str = Path.Combine(godotProjectDir, ".mono", "metadata"); https://www.nuget.org/packages/GodotTools.IdeMessaging

In the Godot support plugin for Rider, I'd like to directly use the nuget.

For now we are stuck with a copy of the sources https://github.com/JetBrains/godot-support/blob/2bdda1af20cc30fb0695cf731f460bb3161c83d7/resharper/GodotTools.IdeMessaging/Client.cs#L124 Which is inconvenient.

To consider

We may not only release a new version from the current source, but also think of how to fix a scenario to support both Godot 3.x and 4.x. https://github.com/JetBrains/godot-support/issues/113

van800 commented 6 months ago

cc: @jsbeckr

raulsntos commented 6 months ago

We can make a new release but naturally that won't work with 3.x. Can you use different versions of this package for 3.x and 4.x?

van800 commented 6 months ago

We can make a new release but naturally that won't work with 3.x. Can you use different versions of this package for 3.x and 4.x?

No. That is unikely.

What if we change the signature of Client ctor? https://github.com/godotengine/godot/blob/3314f8cc6532af5057738d1059876706ee7062af/modules/mono/editor/GodotTools/GodotTools.IdeMessaging/Client.cs#L120 Instead of godotProjectDir, supply it with projectMetadataDir. This way users of the library would have flexibility to support both Godot3 and Godot4. I can make a pull request, if you agree.

raulsntos commented 6 months ago

The problem with that is users of the library would need to know the directory where Godot is storing these internal files that users shouldn't need to know about.

We could change the implementation to check for the old path as a fallback, but I'm not sure if we can guarantee that everything else will work fine with only changing the path to the file.

van800 commented 6 months ago

Adding fallback would work. I have seen several confirmations that coping the metadata file to the old location just works. I can also verify that, if we agree to proceed.

I agree backward compatibility may be broken in the future. I think in our specific case, complexity of supporting multiple versions would overweight the potential benefit.

van800 commented 6 months ago

I would prefer to add optional parameter Godot version to the Client ctor signature. WDYT?

raulsntos commented 6 months ago

I think that solutions like adding a fallback to the old path and adding the Godot version as a parameter of the Client ctor all somewhat imply that the GodotTools.IdeMessaging package supports 3.x.

I'm not sure we want to do that because it would increase the maintainability burden, we would have to ensure that future development doesn't break 3.x support. We also don't maintain compatibility with older major versions in our C# packages, so this would be a bit odd.

I think we can only guarantee that newer versions of GodotTools.IdeMessaging will support 4.x. If we want to add some support for 3.x it will have to be on a best effort basis which means you get no guarantees if it breaks in the future. Is that a reasonable compromise?

If so, I think adding a fallback to the old path would be the best option because adding a Godot version parameter makes it look like other versions are supported and, like I said, we don't want to make any guarantees.

van800 commented 6 months ago

I think that solutions like adding a fallback to the old path and adding the Godot version as a parameter of the Client ctor all somewhat imply that the GodotTools.IdeMessaging package supports 3.x.

I'm not sure we want to do that because it would increase the maintainability burden, we would have to ensure that future development doesn't break 3.x support. We also don't maintain compatibility with older major versions in our C# packages, so this would be a bit odd.

I think we can only guarantee that newer versions of GodotTools.IdeMessaging will support 4.x. If we want to add some support for 3.x it will have to be on a best effort basis which means you get no guarantees if it breaks in the future. Is that a reasonable compromise?

If so, I think adding a fallback to the old path would be the best option because adding a Godot version parameter makes it look like other versions are supported and, like I said, we don't want to make any guarantees.

That makes sense, thank you for the explanation. Should I proceed with making a PR with only adding a fallback?

raulsntos commented 6 months ago

Yes, go ahead.

van800 commented 6 months ago

Tried that https://github.com/van800/godot/commit/5779c6b06dfeecbf0bd6ac247808f37897ec08e3 But unfortunately it doesn't look good to me. If non of the folders exist, then ".godot/mono/metadata" is created and would be monitored. I am about to drop the idea of adding a fallback, instead I might monitor the folder ./mono/metadata in my plugin and reflect its changes to ".godot/mono/metadata".

Please just release the nuget version 1.1.2 as is to the nuget.org.

van800 commented 6 months ago

@raulsntos would you please release the nuget 1.1.2?

raulsntos commented 6 months ago

Sorry for the wait, version 1.1.2 should be out now.

van800 commented 6 months ago

Thank you!

raulsntos commented 6 months ago

Closing since we have now released a new version and it doesn't look feasible to support 3.x in the GodotTools.IdeMessaging package as per https://github.com/godotengine/godot-proposals/issues/9502#issuecomment-2053691712.