Low-hanging nullable fruit #5186
Conversation
Needed for a telemetry summation in extension.ts to function properly.
No consumer uses Range.
* Change optional properties to nullable to better reflect their status * Simplify nullable config retrieval * Simplify readUseGlobalMonoOptions
There was a problem hiding this comment.
Thanks for these changes. Enabling more of the TS compilers strict checking is certainly a goal for us. As well as moving from tslint to eslint.
There are a few failing unit tests that have checks like expect(options.path).to.be.null that will need to be updated.
winstliu
force-pushed
the
low-hanging-nullable-fruit
branch
from
1acc7dc
to
6576c95
Compare
There was a problem hiding this comment.
Ready for review again. Changed all the string-related settings to default to the empty string rather than having to deal with TypeScript warning that it could be undefined. The only time it could be undefined is if the config is removed from package.json, which I think is highly unlikely without corresponding code changes...
| // True means 'always' and false means 'auto'. | ||
| return value ? "always" : "auto"; | ||
| } | ||
|
|
||
| if (omnisharpConfig.has('useGlobalMono')) { |
There was a problem hiding this comment.
I'm not confident this fallback logic ever worked...I'm running the singleCsproj integration test suite right now which doesn't have useGlobalMono defined, but this still returns true (with get returning "auto", the default).
I suppose the upside is this whole section is going away soon, so I don't suppose it matters too much.
There was a problem hiding this comment.
This bit of code will be removed prior to the next release to address #5120
| // True means 'always' and false means 'auto'. | ||
| return value ? "always" : "auto"; | ||
| } | ||
|
|
||
| if (omnisharpConfig.has('useGlobalMono')) { |
There was a problem hiding this comment.
This bit of code will be removed prior to the next release to address #5120
There's a bunch of low-hanging fruit to get variables properly classified as nullable (and null-checked).
This pass gets most of src/omnisharp strict-clean, with the exception of server.ts and anything that depends on IHostExecutableResolver.
I'm hoping most of the changes should be self-explanatory, let me know if anything looks suspicious. Any changes to functionality are unintentional.
Also let me know if you'd prefer not to receive these types of PRs, because I get that they change a lot of files and reviewing is probably a bit of a small headache.