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

Clear all strict mode violations in src & enforce strict mode #5407

Merged

Conversation

winstliu
Copy link
Contributor

@winstliu winstliu commented Oct 17, 2022

First, the remaining strict changes:

  • IGetMonoVersion's contract was changed to allow an undefined return (the callers were already expecting this)
  • Aligned DotnetResolver & MonoResolver:
    • DotnetResolver: Minimum version is now a readonly string instance variable
    • MonoResolver: Environment is handled inline in getHostExecutableInfo
  • Created a new type, SpawnedChildProcess, to denote child processes that have successfully spawned and thus have stderr/in/out & pid properties
  • ❗ Revamped OmnisharpManager/GetOmniSharpLaunchInfo to return just a launch path, rather than 3 launch paths for different scenarios. There are no functional changes (and you'll see that reflected in the test updates) because each scenario only ever used one (non-overlapping) launch path, but it is an involved change so I'll walk through my logic here:
    1. Let's start with DotnetLaunchPath. This path is only set when the extension is configured to use the .NET version of OmniSharp. It's also the only path set in that case.
    2. Looking at the only place that uses DotnetLaunchPath, launcher.launchDotnet, that method first checks to see if LaunchPath is set AND doesn't end in .dll. However, we can see from above that will never be the case, since LaunchPath is never set. Then, it determines the command by first looking at DotnetLaunchPath and only falling back to LaunchPath if that doesn't exist. Again, that fallback will never execute.
    3. Thus, since launchDotnet, the only place that reads from DotnetLaunchPath, does not depend on any of the other launch paths, we can remove DotnetLaunchPath and replace its usage with LaunchPath.
    4. Okay, that leaves MonoLaunchPath. This one is used exclusively on non-Windows platforms when launching .NET Framework OmniSharp. Unlike DotnetLaunchPath, we also set LaunchPath in this scenario.
    5. Now, looking at the caller, launcher.launchNix, we can see that it determines the launch path by first looking for MonoLaunchPath, and falling back to LaunchPath if that doesn't exist. However, once again that fallback will never occur given that MonoLaunchPath will always be set.
    6. Thus, MonoLunchPath can also be removed and its usage replaced by LaunchPath.
    7. Now we have a unified LaunchPath, but there's no point in having it as the sole property of an interface, so convert everything to work directly with the string itself instead.
  • Some other minor changes, like piping workspaceFolders through a few methods after it's been checked for nullability

Second, the tsconfig.json/package.json/test changes:

  • The top-level tsconfig now has strict mode enabled 🎉 (plus forceConsistentCasingInFileNames because the Edge Devtools extension was complaining about that)
    • This tsconfig handles everything but the test folder
  • A new tsconfig.json has been created in the test folder that has strict mode disabled, as there's still ~190 failures in that folder
  • All scripts that deal with tests have been updated to use the test tsconfig
  • Unit tests now run on the compiled JS version because I couldn't figure out how to get ts-loader to pick up the test tsconfig
  • Similarly, tslint now runs directly rather than through gulp because gulp-tslint was trying to use its own set of files that were inconsistent with the tsconfig (this does mean tests are temporarily skipped while linting)

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all this strictness work! this will be fun to merge into feature/lsp-engine. =)

@JoeRobich JoeRobich merged commit 08d4eca into dotnet:master Oct 27, 2022
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

Successfully merging this pull request may close these issues.

2 participants