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

chore(null): strict null check fixes #5457

Merged
merged 17 commits into from
May 17, 2022

Conversation

madalynrose
Copy link
Contributor

@madalynrose madalynrose commented May 5, 2022

Details

This PR took all the yarn null:find results that were resolvable with some extra digging (verifying values will not be falsy when passed into functions, mostly), added them to tsconfig.strictNullChecks.json, fixed the resulting errors, and cleaned up with yarn null:autoadd after.

yarn null:progress before: 66% complete (995/1515 non-test files)
yarn null:progress after: 66% complete (1002/1516 non-test files)

Motivation

#2869

Context

Adding inline comments to this PR where context is needed.

Pull request checklist

  • Addresses an existing issue: Codebase should use strict null checks #2869
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.

@madalynrose madalynrose requested a review from a team as a code owner May 5, 2022 17:34
@@ -39,7 +39,7 @@ export class FailedInstancePanel extends React.Component<FailedInstancePanelProp
label="Comment"
multiline={true}
rows={8}
value={this.props.instanceDescription}
value={this.props.instanceDescription ?? ''}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The TextField component expects a string.

@@ -47,8 +47,8 @@ export const TabStopsFailedInstancePanel = NamedFC<TabStopsFailedInstancePanelPr
confirmButtonText: 'Add failed instance',
onConfirm: () => {
tabStopRequirementActionMessageCreator.addTabStopInstance({
description: failureInstanceState.description,
requirementId: failureInstanceState.selectedRequirementId,
description: failureInstanceState.description!,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only called when the form is submitted, and the form can only be submitted once values for these attributes are provided.

@@ -73,8 +73,8 @@ export class ScanController {
const payload = this.unifiedResultsBuilder(data);

this.unifiedScanResultAction.scanCompleted.invoke(payload);
this.scanActions.scanCompleted.invoke(null);
this.deviceConnectionActions.statusConnected.invoke(null);
this.scanActions.scanCompleted.invoke();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

passing null vs void does not alter the behavior of the invocation so we should call the function without params as intended

@@ -125,7 +125,7 @@ export class ConvertScanResultsToUnifiedResults {
snippet: nodeResult.snippet || nodeResult.html,
},
resolution: {
howToFixSummary: nodeResult.failureSummary,
howToFixSummary: nodeResult.failureSummary!,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way we have architected our Unified Results ensures that every rule has an associated failureSummary.

@@ -60,7 +60,7 @@ export class FocusChangeHandler {
}

private findAutomatedChecksFocusedResult(storeData: TargetPageStoreData): UnifiedResult {
const focusedResult = storeData.unifiedScanResultStoreData.results.find(
const focusedResult = storeData.unifiedScanResultStoreData.results!.find(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These functions are only called once we have results.

Copy link
Contributor

@peterdur peterdur left a comment

Choose a reason for hiding this comment

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

LGTM!
Reviewed code, built locally, quick smoke run of AI-Web and AI-Android

screenshotData?: ScreenshotData;
results: UnifiedResult[] | null;
rules: UnifiedRule[] | null;
platformInfo?: PlatformData | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

It reads a little strange to have thing?: Type | null but I still like ? for optional fields...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

@madalynrose madalynrose merged commit d6753ff into microsoft:main May 17, 2022
@madalynrose madalynrose deleted the null-checks-larger-changes branch May 17, 2022 15:05
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