OmniSharp / omnisharp-vim

Vim omnicompletion (intellisense) and more for C#
http://www.omnisharp.net
MIT License
1.7k stars 169 forks source link

Improve vimspector config file generation #747

Closed Melandel closed 2 years ago

Melandel commented 2 years ago

Currently:

  1. the working directory is not set which means access to runtime config files may or may not be possible
  2. on windows, the targetPath is expressed with non-escaped backslashes, which makes the json value not the one the user wants
nickspoons commented 2 years ago

Thanks @Melandel

I'm away from the computer for the next few days and can't test anything. @jpfeiffer16 are you happy with this?

jpfeiffer16 commented 2 years ago

@nickspoons I'll test it out sometime today.

Melandel commented 2 years ago

If you know a good way to translate \ paths into / paths, I would be absolutely interested.

I am currently using substitute(path, '\', '/', 'g') but it has obvious limitations...

jpfeiffer16 commented 2 years ago

@Melandel . I think there is some code in the project somewhere for converting cigwin to windows style paths. I would think that this is exactly what it is doing.

jpfeiffer16 commented 2 years ago

@Melandel , @nickspoons this works as advertised. I have one concern/question though which is this: Wouldn't this break workflows where you are trying to, for example access a file relative to the root of your project. An example of this would be a program that takes a text file as input you would typically run it like so dotnet run ./test-input-file.txt If you are setting the cwd to the bin directory, then this way of "running the program" in my current working directory is changed and it would not be able to find the file unless given an absolute path, or the file is marked for copy-to-output. I say this because the way that vscode for example sets up it's launch.json files is so that this "run in the current directory" behavior still works as expected when run in the debugger by default.

So, that would lead me to believe there is an additional reason for having this behavior that I don't understand:

the working directory is not set which means access to runtime config files may or may not be possible

Does this mean that if you debug the project with the CWD set to something other than the bin directory containing the executable that it may not work in some cases?

Melandel commented 2 years ago

@jpfeiffer16

Does this mean that if you debug the project with the CWD set to something other than the bin directory containing the executable that it may not work in some cases?

In previous experiences

I did witness the program throwing at the start of its execution, not able to find configuration files such as App.exe.config or appsettings.json for instance - unless it was started from the folder containing the executable.

I am not too used to configuration shenanigans, and in my experience people use this kind of code to setup configuration files:

public Startup(IConfiguration configuration, IHostingEnvironment env)
{
      var builder = new ConfigurationBuilder()
         .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true) // looks for the file in cwd
         .AddEnvironmentVariables();

      Configuration = builder.Build();
}

However

In order to write this post, I took a look at how configuration was handled nowadays and saw that starting with dotnet core 2.1, there is a notion of ContentRoot which determines where the host begins searching for content files and is not the same notion as Current Directory (Directory.GetCurrentDirectory())

I believe that in my previous experiences, the source code was either .NET Framework, either did not use ConfigurationBuilder.SetBasePath(env.ContentRootPath), resulting in the configuration files such as App.exe.config or more recently appsettings.json having to be inside the current working directory.

In short, for my next experiences, I'll start using this line

public Startup(IConfiguration configuration, IHostingEnvironment env)
{
      var builder = new ConfigurationBuilder()
         .SetBasePath(env.ContentRootPath)  // here
         .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
         .AddEnvironmentVariables();

      Configuration = builder.Build();
}

In conclusion

Unfortunately not:

let g:OmniSharp_translate_cygwin_wsl=1 echo OmniSharp#util#TranslatePathForClient('C:\Users\tranm\Desktop\tools\vim\pack\plugins\start\omnisharp-vim\README.md')

C:\c\Users\tranm\Desktop\tools\vim\pack\plugins\start\omnisharp-vim\README.md

I'll keep my substitute as is for now!

jpfeiffer16 commented 2 years ago

@Melandel . Thank you for looking into all of that. @nickspoons this looks good to go as far as I am concerned.

nickspoons commented 2 years ago

Thanks very much @Melandel, and @jpfeiffer16 for the review!