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

FED-756 Allow analyzer 2.x, fix analyzer plugin in newer SDKs #807

Merged
merged 14 commits into from Feb 16, 2023

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Feb 13, 2023

Motivation

We want to pull in newer analyzer versions, and unlocking analyzer 2.x is the next step.

Also, while testing I noticed that the analyzer plugin doesn't correctly start in IDEs in Dart 2.18 ☹️. Since this PR touches analyzer_plugin, I figured I'd fix that so we can manually verify changes.

Changes

  • Widen analyzer range to allow 2.x
  • Reinstate validate_analyzer CI check to ensure over_react is compatible with both 1.x and 2.x
  • Mockito
    • Pin mockito (in dev_dependencies) to work around compilation issue
    • Remove unnecessary List mock, which was causing build failures (more info in 578132d)
  • Upgrade analyzer plugin to analyzer 2.x.
    Supporting both isn't necessary since the plugin is its own separate dependency graph, and supporting one is almost definitely easier. I went ahead and picked 2.x as the one to resolve to, since we'd have to upgrade anyways whenever the main package drops support for 1.x.
    • Fix warnings in caused by upgrade (mostly removal of non-null assertions, since analyzer 2.x promoted some APIs to non-nullable)
    • Copy IgnoreInfo into package, since we were previously importing it from analyzer via a src import, and the API changed to take in ErrorCode objects instead of Strings, and we couldn't construct ErrorCodes without more src imports
    • Fix low-hanging deprecation warnings
    • Disable ToggleComponentStatefulness, BoilerplateValidatorDiagnostic, and IncorrectDocCommentLocationDiagnostic ☹️.
      These all import non-null-safe code from over_react's boilerplate parsing, which apparently prevents the plugin from being able to run in 2.18 SDK, and our previous workaround of a language comment in the plugin's main no longer works. Until we're able to migrate that code to null safety (which will hopefully be soonish), we'll need to stop importing these to get the plugin working again.

Release Notes

Widen analyzer range to allow 2.x in addition to 1.x

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code
  • Manual testing was performed if needed
    • Steps from PR author:
      • All CI passes , notably the Dart CI / validate_analyzer (2.18.7, ^2.0.0) run
      • Verify analyzer plugin works in playground in Dart 2.18
    • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

  • A Frontend Frameworks Design member has reviewed these changes
  • There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
  • For release PRs - Version metadata in Rosie comment is correct

@aviary3-wk
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

Errors look like:

  Invalid @GenerateMocks annotation: Mockito cannot generate a valid mock class which implements 'List' for the following reasons:
    The method 'Iterable.reduce' features a non-nullable unknown return type, and cannot be stubbed. Try generating this mock with a MockSpec with 'unsupportedMembers' or a dummy generator (see https://pub.dev/documentation/mockito/latest/annotations/MockSpec-class.html).
channel.sendNotification(plugin.AnalysisErrorsParams(analysisResult.path!, []).toNotification());
return [];
}

Copy link
Member

Choose a reason for hiding this comment

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

I assume this was removed because analysisResult.unit is non-nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, exactly.

robbecker-wf
robbecker-wf previously approved these changes Feb 14, 2023
@greglittlefield-wf greglittlefield-wf changed the title Allow analyzer 2.x Allow analyzer 2.x, fix analyzer plugin in newer SDKs Feb 14, 2023
@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review February 14, 2023 21:42
@rmconsole6-wk rmconsole6-wk changed the title Allow analyzer 2.x, fix analyzer plugin in newer SDKs FED-756 Allow analyzer 2.x, fix analyzer plugin in newer SDKs Feb 14, 2023
Copy link
Contributor

@hunterbreathat-wk hunterbreathat-wk left a comment

Choose a reason for hiding this comment

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

While I may not be overly familiar with analyzers, these changes make sense; many of them are null-safe changes. I've got a couple of easy questions and a nit.

@robbecker-wf
Copy link
Member

QA+1 CI Passes with updated tests
We walked through using the analyzer plugin in a project and verified that it is using analyzer 2 and functions as it should.

@greglittlefield-wf
Copy link
Contributor Author

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

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

Successfully merging this pull request may close these issues.

None yet

6 participants