Skip to content

Add Dart Code Metrics #2055

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

Merged
merged 12 commits into from
Mar 30, 2023
Merged

Add Dart Code Metrics #2055

merged 12 commits into from
Mar 30, 2023

Conversation

elliette
Copy link
Contributor

@elliette elliette commented Mar 29, 2023

Work towards #2053

Applies fixes for rules that are trivial to fix. Rules with non-trivial fixes are commented out, with TODO.

@elliette elliette marked this pull request as ready for review March 29, 2023 20:05
@elliette elliette changed the title [Do not submit] Add Dart Code Metrics Add Dart Code Metrics Mar 29, 2023
@elliette elliette requested a review from annagrin March 29, 2023 20:05
elliette added a commit to elliette/webdev that referenced this pull request Mar 29, 2023
Copy link
Contributor

@annagrin annagrin left a comment

Choose a reason for hiding this comment

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

LGTM!

@elliette elliette merged commit 0afc9eb into dart-lang:master Mar 30, 2023
- avoid-unrelated-type-assertions
- avoid-unused-parameters
- binary-expression-operand-order
- double-literal-format
Copy link
Member

Choose a reason for hiding this comment

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

can you include all of the lint rules for DCM here with the ones you have not enabled commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added changes for this (and other PR comments) at #2072

@@ -62,7 +62,7 @@ void _registerListeners() {
Future<void> _handleRuntimeMessages(
dynamic jsRequest,
MessageSender sender,
Function sendResponse,
Function _,
Copy link
Member

Choose a reason for hiding this comment

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

not sure this change is an improvement. consider using an ignore comment instead. DCM should be able to figure out that the arg is needed based on where this is used as a listener.

@@ -29,7 +29,7 @@ final _eventsForAngularDartDevTools = {

Future<void> handleMessagesFromAngularDartDevTools(
dynamic jsRequest,
MessageSender sender,
MessageSender _,
Copy link
Member

Choose a reason for hiding this comment

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

consider an ignore comment here as well.

@@ -107,7 +107,8 @@ void _forwardMessageToAngularDartDevTools(ExternalExtensionMessage message) {
chrome.runtime.sendMessage(
_angularDartDevToolsId,
message,
/* options */ null,
/* options */
Copy link
Member

Choose a reason for hiding this comment

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

I would switch this to a // comment
odd we don't have any lints to warn about /* comments within a method body

@@ -234,9 +234,11 @@ _enableExecutionContextReporting(int tabId) {
if (chromeError != null) {
final errorMessage = _translateChromeError(chromeError.message);
chrome.notifications.create(
/*notificationId*/ null,
/*notificationId*/
Copy link
Member

Choose a reason for hiding this comment

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

same as before. Switch these to // comment

@@ -42,7 +42,7 @@ Future<Tab?> getTab(int tabId) {
return completer.future;
}

Future<Tab?> get activeTab async {
Future<Tab?> get activeTab {
Copy link
Member

Choose a reason for hiding this comment

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

i keep finding it interesting how many cases this lint catches.

@@ -619,6 +616,12 @@ class AppInspector implements AppInspectorInterface {
});
}

Iterable<String> _userLibraryUris(Iterable<LibraryRef> libraries) {
Copy link
Member

Choose a reason for hiding this comment

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

what lint caused this refactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I think this snuck in when I was trying to address:

metrics:
   cyclomatic-complexity: 20
   number-of-parameters: 5
   maximum-nesting-level: 5

before disabling those checks as a follow-up. Forget to revert the changes here.

Copy link
Member

@jacob314 jacob314 left a comment

Choose a reason for hiding this comment

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

lgtm

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.

3 participants