Skip to content
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

Support XDG directory specification? #511

Closed
DianeOfTheMoon opened this issue Sep 9, 2019 · 6 comments
Closed

Support XDG directory specification? #511

DianeOfTheMoon opened this issue Sep 9, 2019 · 6 comments

Comments

@DianeOfTheMoon
Copy link
Contributor

Given the fact that Neovim uses the xdg directory specification should OmniSharp-vim also support it, if it's detected? I think where the server is installed is about the only item that would be updated.

@nickspoons
Copy link
Member

Perhaps ... the reason that OmniSharp-vim installs OmniSharp-roslyn under ~/.omnisharp is because OmniSharp-roslyn already uses that directory, that's where ~/.omnisharp/omnisharp.json is.

Related: OmniSharp/omnisharp-roslyn#953

@nickspoons
Copy link
Member

@DianeOfTheMoon I just noticed OmniSharp/omnisharp-roslyn#1317 which checks for an $OMNISHARPHOME environment variable for the omnisharp.json file. Using that would be pretty simple, but I suppose we could check for any of $OMNISHARPHOME, $XDG_DATA_HOME, ~/.local/share and use any of those if they exist - does that sound reasonable?

Were you considering making a PR? Note that this involves both installing the server and starting it.

@DianeOfTheMoon
Copy link
Contributor Author

DianeOfTheMoon commented Sep 10, 2019

Yeah, I'd put up a PR, just wanted to get your read before I did. $XDG_DATA_HOME, then $OMNISHARPHOME, then default to $HOME/.omnisharp? I'll try and consolidate the path construction in the process...sound good?

@DianeOfTheMoon
Copy link
Contributor Author

DianeOfTheMoon commented Sep 10, 2019

@nickspoons Since I've never really done anything with Vimscript, does this look right to you? It should not break existing installs, but prefer XDG for neovim installations:

if exists('g:OmniSharp_server_path')
  let g:OmniSharp_server_home = fnamemodify(g:OmniSharp_server_path, ':p:h')
else
  if exists('$OMNISHARPHOME')
    let g:OmniSharp_server_home = $OMNISHARPHOME
  endif
endif

if !exists('g:OmniSharp_server_home')
  " We should start by seeing if someone already has an install or basic
  " config
  let dir_separator = fnamemodify('.', ':p')[-1 :]
  let root_path = [expand('$HOME'), '.omnisharp']
  let root_dir = join(root_path, dir_separator)
  let settings_file = join(root_path + ['omnisharp.json'], dir_separator)

  if filereadable(settings_file) || isdirectory(root_dir) || !has('nvim')
    let g:OmniSharp_server_home = root_dir
  else
    " Neovim uses the XDG directory specification, so we should, too
    if has('win32')
      let data_home_default = expand('%APPDATALOCAL%')
    else
      let data_home_default = join([expand('$HOME'), '.local', 'share'], dir_separator)
    endif
    let data_home = exists('$XDG_DATA_HOME') ? $XDG_DATA_HOME : data_home_default
    let g:OmniSharp_server_home = join([data_home, 'omnisharp'], dir_separator)
  endif
endif

Sorry to be a bit gun shy, but want to validate this before updating anything else and I don't know if I'm over complicating it.

@nickspoons
Copy link
Member

@DianeOfTheMoon looks good!

  1. I don't think we should treat Vim and Neovim differently.
  2. I believe APPDATALOCAL should be LOCALAPPDATA, and in my Windows 10 gvim echo expand('%LOCALAPPDATA%') doesn't output anything, but echo expand('$LOCALAPPDATA') does.
  3. For the sake of consistency, lets always wrap environment variables with expand() (there are unwrapped $OMNISHARPHOME and XDG_DATA_HOME).

If you want to get something up that's easier to collaborate on, just bang up a PR with "WIP" in the title - I won't merge it before you give me the go-ahead. That way it's easier to comment on the code and see it in context (I don't have a problem with doing it here, just a suggestion because the PR system works well).

@nickspoons
Copy link
Member

Closed by #513

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants