Thanks for the PR.
For all the SkipTest( 'Reason' ) chanages you should have used @ExpectedFailure from test_utils.py.

What is preventing us from having a working force_semantic?

Reviewed 12 of 15 files at r1, 5 of 5 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @mispencer and @ohjames)

ycmd/completers/cs/cs_completer.py, line 74 at r3 (raw file):

      raise RuntimeError(
           SERVER_NOT_FOUND_MSG.format( self._omnisharp_path ) )

Extra empty line.

ycmd/completers/cs/cs_completer.py, line 111 at r3 (raw file):

    enough to do so and it will returns more relevant results. Fallback to use
    the triggers, which are by default . -> and :: """
    if ( self.QueryLengthAboveMinThreshold( request_data ) ):

Unnecessary parens. Also why not just

return ( self.QueryLengthAboveMinThreshold( request_data ) or
         super( CsharpCompleter, self ).ShouldUseNowInner( request_data ) )

ycmd/completers/cs/cs_completer.py, line 195 at r3 (raw file):

      #                              no_request_data = True ) ),
      # 'SetOmnisharpPath'                 : ( lambda self, request_data, args:
      #    self._SetOmnisharpPath( request_data, args[ 0 ] ) ),

Debugging artifacts?

ycmd/completers/cs/cs_completer.py, line 330 at r3 (raw file):

  def _SetOmnisharpPath( self, request_data, omnisharp_path ):

Is this used anywhere?

ycmd/completers/cs/cs_completer.py, line 652 at r3 (raw file):

    target = urljoin( self._ServerLocation(), handler )
    LOGGER.info( u'Sending request' )
    response = requests.post( target,

This should be possible to fit on one line.

ycmd/tests/cs/diagnostics_test.py, line 143 at r3 (raw file):

@SharedYcmd
def Diagnostics_HandleZeroColumnDiagnostic_test( app ):
  raise SkipTest( "No zero column diagnostic in roslyn" )

We could just remove the test if we are sure of this.

Reviewed 1 of 15 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @mispencer and @ohjames)

build.py, line 703 at r3 (raw file):

      continue
    if os.path.isfile( file_path ):
      os.unlink( file_path )

I'd use os.remove here, because unlink makes me think of symbolic links.

build.py, line 753 at r3 (raw file):

      if OnMac():
        return 'omnisharp.http-osx.tar.gz'
      elif platform.machine().endswith( '64' ):

No need for elif or else after return.

The tests with SkipTest should either have be removed, or the test should made to work. In either case, they wouldn't have either SkipTest or ExpectedFailure, so I'm not going to worry about this for now.

The old Omnisharp server had two complete modes: a complete-as-you-type mode and explicitly requested mode (i.e you press a keybind like shift-tab). Omnisharp-roslyn doesn't have this feature as far as I am aware, thus I removed the force feature and all the tests for it.

The import feature and its tests were removed for the same reason.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @ohjames)

build.py, line 753 at r3 (raw file):

Done.

ycmd/completers/cs/cs_completer.py, line 74 at r3 (raw file):

Done.

ycmd/completers/cs/cs_completer.py, line 111 at r3 (raw file):

Done.

ycmd/completers/cs/cs_completer.py, line 195 at r3 (raw file):

I left this here because it was useful for development work on the C# completer, but commented it out since it should not be exposed for normal users. I had originally implemented this prior to omnisharp-roslyn for my own development and can confirm it was useful for me. If you want, however, I can remove the whole variable omnisharp server path feature.

ycmd/completers/cs/cs_completer.py, line 330 at r3 (raw file):

It isn't exposed anywhere at the moment, but same comment as on the SetOmnisharpPath command applied here.

ycmd/completers/cs/cs_completer.py, line 652 at r3 (raw file):

Done.

ycmd/tests/cs/diagnostics_test.py, line 143 at r3 (raw file):

I am unaware of any way to get a zero-column from Omnisharp-roslyn, but I cannot say for sure.

The Travis failure looks like a flake, so this is mostly ready. There are a few items left to clean up (see my other notes), but this is ready for review again.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @ohjames)

build.py, line 723 at r4 (raw file):

USE_MONO_PACKAGE = False

Need to figure out if we are supporting building using the mono version. If so, need to ensure that the mono version works, and probably should have one of the Travis configuration use it. Else, this feature should be removed, not just flagged out.

run_tests.py, line 260 at r4 (raw file):

    env[ 'PATH' ] = LIBCLANG_DIR + ';' + env[ 'PATH' ]
  elif 'LD_LIBRARY_PATH' in env:
    env[ 'LD_LIBRARY_PATH' ] = env[ 'LD_LIBRARY_PATH' ] + ':' + LIBCLANG_DIR

This can be removed.

ycmd/tests/cs/diagnostics_test.py, line 183 at r4 (raw file):

  app.post_json( '/event_notification', event_data ).json
  WaitUntilCompleterServerReady( app, 'cs' )
  time.sleep( 5 )

Replace this with the logic in __init__. Probably should be refactored to a function and used in both places.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @ohjames)

ycmd/tests/cs/init.py, line 103 at r4 (raw file):

    shared_filepaths.append( filepath )
    WaitUntilCompleterServerReady( app, 'cs' )
    time.sleep( 2 )

This sleep can likely be removed.

The old Omnisharp server had two complete modes: a complete-as-you-type mode and explicitly requested mode (i.e you press a keybind like shift-tab). Omnisharp-roslyn doesn't have this feature as far as I am aware, thus I removed the force feature and all the tests for it.

I'm not sure I understand. We're still sending the /autocomplete request to the Roslyn server. So for force_semantic to work, we just need to have another or request_data[ 'force_semantic in ShouldUseNowInner().

The Travis failure looks like a flake, so this is mostly ready. There are a few items left to clean up (see my other notes), but this is ready for review again.

It definitely was a flake, I restarted it and the tests are passing. Mark the PR as [READY], otherwise it won't get as much attention.

Reviewed 9 of 10 files at r4.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @mispencer and @ohjames)

.travis.yml, line 3 at r4 (raw file):

language: generic
dist: trusty
sudo: false

Why was this removed? I see that you're installing libuv isn't just adding it to the Travis list of packages: enough?

build.py, line 673 at r1 (raw file):

That would be great.

build.py, line 723 at r4 (raw file):

Did I understand correctly? Are we talking about building mono from source or something else?

ycmd/completers/cs/cs_completer.py, line 111 at r3 (raw file):

Align super with self.

ycmd/completers/cs/cs_completer.py, line 195 at r3 (raw file):

There's the debug info, the logger and one can use an actual debugger if needed. I'd prefer a code without "might be useful one day for debugging".

ycmd/tests/cs/init.py, line 103 at r4 (raw file):

That would be nice, since the tests already take a while to run.

ycmd/tests/cs/init.py, line 112 at r4 (raw file):

      except Exception:
        if i == 19: # i is zero indexed
          raise

Let's have a real description of the error that happened.

ycmd/tests/cs/diagnostics_test.py, line 143 at r3 (raw file):

A SkipTest() for something that we don't know if it can happen doesn't seem useful to me.

I'm not sure I understand. We're still sending the /autocomplete request to the Roslyn server. So for force_semantic to work, we just need to have another or request_data[ 'force_semantic in ShouldUseNowInner().

The old Omnisharp would return different result in forced and non-forced mode. This is why that logic to divide the cache and such exists. The new Omnisharp returns the same result, so none of that logic and related are necessary, and thus I removed them.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @ohjames)

.travis.yml, line 3 at r4 (raw file):

"sudo: false" was used to opt in to the Travis container environment, but that no longer exists, so it is just a no op. As such, we can use sudo, and I did in the install script. Thus, I removed the "sudo: false" no op, since it would be confusing.

build.py, line 673 at r1 (raw file):

Done.

build.py, line 723 at r4 (raw file):

No. Omnisharp has two different linux/mac packages. One only includes Omnisharp and the other also bundles a Mono instance. We are currently using the package that bundles Mono. My earlier notes were wondering if we should support using the bare package and thus the system mono.

run_tests.py, line 260 at r4 (raw file):

Done

ycmd/completers/cs/cs_completer.py, line 111 at r3 (raw file):

Done

ycmd/completers/cs/cs_completer.py, line 195 at r3 (raw file):

The use case for this was using a different Omnisharp binary on the fly. Regardless, I have removed the variable Omnisharp server path feature.

ycmd/tests/cs/init.py, line 103 at r4 (raw file):

Done

ycmd/tests/cs/init.py, line 112 at r4 (raw file):

Not sure what you mean? I refactored this code, so please review it there, and re-comment if need be.

ycmd/tests/cs/diagnostics_test.py, line 143 at r3 (raw file):

I removed this test, since I am not aware of any such response from Omnisharp-roslyn.

ycmd/tests/cs/diagnostics_test.py, line 183 at r4 (raw file):

Done

Nice one. Gave it a really quick review. Will need to review more thoroughly .

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic, @mispencer, and @ohjames)

build.py, line 723 at r4 (raw file):

Let’s just pick one and drop the extra unreachable code.

build.py, line 264 at r5 (raw file):

def CheckOutput( args, **kwargs ):

What’s this needed for?

build.py, line 690 at r5 (raw file):

    package_path = p.join( version, url_file )
    if not p.exists( package_path ):
      DownloadFileTo( url_pattern.format( version, url_file ), package_path )

We should do a checksum validation on any existing file like jdt and clangd completers

build.py, line 693 at r5 (raw file):

    if OnWindows():
      extract_command = [ GetSevenZipPath(), 'x', package_path ]

Can we not use the python stuff that is used by jdt.ls installer ? Requiring 7zip is a pain I would like to avoid.

build.py, line 702 at r5 (raw file):

def CleanCsCompleter( build_dir, version ):

this seems overly complex. What we do for jdt.ls seems simple enough.

build.py, line 750 at r5 (raw file):

    # TODO - improve this check
    libuv_output = CheckOutput(
      [ 'gcc', '-luv' ], stderr = subprocess.STDOUT )

Gcc might not exist or be in the path. Is the check really needed? Can we just write it in the docs?

build.py, line 763 at r5 (raw file):

def ToUnicode( value ):

What’s this needed for?

ycmd/completers/cs/cs_completer.py, line 419 at r5 (raw file):

      try:
        self._GetResponse( '/stopserver', timeout = .5 )
      except Exception:

Can this be more specific?

ycmd/completers/cs/cs_completer.py, line 427 at r5 (raw file):

  def _ForceStopServer( self ):

We have a utils method for this now I think.

Thanks for the PR. This mostly looks good, but like Ben I will give this PR another thorough look.

Reviewed 5 of 5 files at r5.
Dismissed @ohjames from a discussion.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @mispencer)

.travis.yml, line 3 at r4 (raw file):

Still, is just adding libuv to the packages: enough or do we have to do this kind of acrobatics?

ycmd/tests/cs/init.py, line 112 at r4 (raw file):

I was talking about raise vs raise RuntimeError( "Timeout blah blah blah" ), but since this piece of code is removed, it's fine.

ycmd/tests/cs/init.py, line 101 at r5 (raw file):

    try:
      if len( GetDiagnostics( app, filepath ) ) == 0:
        raise Exception( "No diagnostic" )

Let's instead throw (and catch) RuntimeError.

Updated to latest Omnisharp release (v1.32.13).

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

.travis.yml, line 3 at r4 (raw file):

It wasn't in the source repo whitelist last I checked. I am copying the Omnisharp travis script logic.

build.py, line 723 at r4 (raw file):

We are currently used the bundled one, so I dropped support for the non-bundled one.

build.py, line 264 at r5 (raw file):

Dead code, removed.

build.py, line 690 at r5 (raw file):

I am not interested in implementing this at this time. Maybe later when the version is more stable.

build.py, line 693 at r5 (raw file):

Done.

build.py, line 702 at r5 (raw file):

This logic removed any older versions as well, which is worth the extra couple of lines.

build.py, line 750 at r5 (raw file):

Removed both these checks.

build.py, line 763 at r5 (raw file):

Dead code - removed.

ycmd/completers/cs/cs_completer.py, line 419 at r5 (raw file):

Exception is used in the other similar checks (e.g. ServerIsReady) so I assume not?

ycmd/completers/cs/cs_completer.py, line 427 at r5 (raw file):

I didn't find one?

ycmd/tests/cs/init.py, line 101 at r5 (raw file):

Changed throws. We aren't catching RuntimeError because we need to catch errors thrown by GetDiagnostics, which include types outside of RuntimeError.

Reviewed 4 of 4 files at r6.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @mispencer and @puremourning)

.travis.yml, line 3 at r4 (raw file):

I /love/ travis whitelist...

ycmd/tests/cs/init.py, line 101 at r5 (raw file):

We aren't catching RuntimeError because we need to catch errors thrown by GetDiagnostics, which include types outside of RuntimeError.

Then let's be explicit and name all exception types that we want to catch, instead of just catching everything and potentially hide a bug.

Updated to latest stable release (v1.32.17)

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

build.py, line 690 at r5 (raw file):

This is implemented, as we are now on a stable release.

ycmd/tests/cs/init.py, line 101 at r5 (raw file):

I don't know all the possible failure cases. The ServerIsReady and ServerIsHealthy checks on the completer are trapping exception as well for the same reason - how the server will fail when it isn't ready isn't well defined.

More generally, for testing purposes, if the server eventually returns successfully, we don't care why it was failing before. If it isn't ever successful, the final exception will be re-raised regardless of the exception type, so we will properly get a test failure with a usefully error message.

@mispencer What's the status of this? Aren't FixIts now possible?

Reviewed 1 of 3 files at r7.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

Alright, if FixIts are going to be a chore, we can leave them for another PR. I left a few minor comments, but generally, .

Thanks a lot for all the work you put into this.

Reviewed 2 of 3 files at r7.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @mispencer and @puremourning)

update_omnisharp.py, line 24 at r7 (raw file):

    if p.isfile( p.join( path, 'os.py' ) ):
      return index
  raise RuntimeError( 'Could not find standard library path in Python path.' )

This is how we used to do this, but it wasn't robust enough.

From the server_utils.py:

def IsStandardLibraryFolder( path ):
  return ( ( p.isfile( path )
             and PYTHON_STDLIB_ZIP_REGEX.match( p.basename( path ) ) )
           or p.isfile( p.join( path, 'os.py' ) ) )


def IsVirtualEnvLibraryFolder( path ):
  return p.isfile( p.join( path, 'orig-prefix.txt' ) )


def GetStandardLibraryIndexInSysPath():
  for index, path in enumerate( sys.path ):
    if ( IsStandardLibraryFolder( path ) and
         not IsVirtualEnvLibraryFolder( path ) ):
      return index
  raise RuntimeError( 'Could not find standard library path in Python path.' )

update_omnisharp.py, line 31 at r7 (raw file):

                                        'requests_deps' ) )
  for path in listdir( request_dep_root ):
    sys.path.insert( 0, p.join( request_dep_root, path ) )

This will add third_party/requests_deps/urllib3 to the path, which isn't a path to a valid python package. I'd prefer to be tidy with things like sys.path and here the easiest way to do that is just to unroll the loop.

update_omnisharp.py, line 53 at r7 (raw file):

  'release': ( "https://github.com/OmniSharp/omnisharp-roslyn/"
               "releases/download/{version}/{file_name}" ),
       'ci': ( "https://roslynomnisharp.blob.core.windows.net/"

Why do we have separate URL for CI?

update_omnisharp.py, line 144 at r7 (raw file):

def Main():
  args = ParseArguments()
  version = args.version

Is there a way to default the version to the latest instead of having to look up what has been released, similar to update_boost.py?

Not really a big deal.

I am reluctant to spend much time polishing the update Omnisharp script, given how rarely it will be ran....

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

update_omnisharp.py, line 24 at r7 (raw file):

I just copied it from the clang update script as I recall. The clangs update script also had this code. If it is an issue, it should be changed in all three places in a separate PR.

update_omnisharp.py, line 31 at r7 (raw file):

That is unlike to cause harm. "Added a request dependency" seems like a more likely failure case.

update_omnisharp.py, line 53 at r7 (raw file):

Release are published to GitHub, but pre-release are only published to the CI url. We have had a need to point at pre-release before, hence why it is supported here.

update_omnisharp.py, line 144 at r7 (raw file):

Probably, but, given how rarely this script will be used, I don't think the benefit is worth the implementation effort.

That likely applies to all possible polishing to this script.

LGTM on the surface. I don't use this completer, though so only scanned it mainly.

Thanks @mispencer for your efforts on this!

Reviewed 8 of 15 files at r1, 5 of 10 files at r4, 1 of 5 files at r5, 2 of 4 files at r6, 2 of 3 files at r7, 1 of 1 files at r8.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @bstaletic and @mispencer)

ci/travis/travis_install.sh, line 131 at r8 (raw file):

make
sudo make install
cd $OLDPWD

use pushd/popd

ycmd/completers/cs/cs_completer.py, line 419 at r5 (raw file):

assumption is the mother of all....

ycmd/completers/cs/cs_completer.py, line 427 at r5 (raw file):

utils.TerminateProcesss or something.

ycmd/completers/cs/cs_completer.py, line 367 at r8 (raw file):

        command.insert( 0, 'mono' )

      LOGGER.info( 'Starting OmniSharp server with: ' + str( command ) )

use LOGGER.info( '.... %s' , command )

ycmd/completers/cs/cs_completer.py, line 618 at r8 (raw file):

    """ Handle communication with server """
    target = urljoin( self._ServerLocation(), handler )
    LOGGER.info( u'Sending request' )

We don't need this, it's too verbose. Consider debug logging that logs the actual message, but just sendin/receiving isn't useful.

Also FWIW the u' is not required, as we use unicode literals from future.

Alright, this gets my .

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic and @mispencer)

Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic and @mispencer)

Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @bstaletic and @mispencer)

I merged master. It looks like Rust is failing in Linux? I don't see anything in these changes which would break Rust tests.

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @bstaletic, @mispencer, and @puremourning)

Reviewed 4 of 6 files at r9, 1 of 1 files at r10, 1 of 1 files at r11.
Dismissed @puremourning from a discussion.
Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @mispencer and @puremourning)

update_omnisharp.py, line 24 at r7 (raw file):

This isn't really an issue, because users won't be running this script anyway.

ycmd/completers/cs/cs_completer.py, line 427 at r5 (raw file):

There's WaitUntilProcessIsTerminated which only waits and eventually raises an exception.

Other than that, I don't see anything useful in utils.py.

ycmd/completers/cs/cs_completer.py, line 526 at r11 (raw file):

  def _GoToReferences( self, request_data ):

Can you add some tests for this? It a great feature, but it adds quite a few lines that currently aren't tested.

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @bstaletic, @mispencer, and @puremourning)

ycmd/completers/cs/cs_completer.py, line 367 at r8 (raw file):

Done.

ycmd/completers/cs/cs_completer.py, line 618 at r8 (raw file):

Done.

ycmd/completers/cs/cs_completer.py, line 526 at r11 (raw file):

Oops, I didn't mean to include this commit in this PR It's been reverted. (This likely will be a later PR.)

@mispencer Can you squash the commits, so that we can merge this?

Reviewed 1 of 1 files at r12.
Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @mispencer and @puremourning)

@mispencer Please check the tests. On linux/python2.7/no libclang build, the Diagnostics_Basic_test is consistently returning no diagnostics.

Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @mispencer and @puremourning)