Skip to content

fix: Linter issues #52

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 11, 2020
Merged

fix: Linter issues #52

merged 4 commits into from
May 11, 2020

Conversation

bruno-garcia
Copy link
Member

With Flutter/Dart:

Flutter 1.12.13+hotfix.9 • channel stable • https://github.com/flutter/flutter.git
Framework • revision f139b11009 (12 days ago) • 2020-03-30 13:57:30 -0700
Engine • revision af51afceb8
Tools • Dart 2.7.2

Running ./tool/presubmit.sh printed:

+ dartanalyzer --fatal-warnings ./
Analyzing sentry-flutter...
  lint • Omit type annotations for local variables. • example/main.dart:18:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • example/main.dart:19:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • example/main.dart:26:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:124:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:129:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:146:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:147:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:153:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:155:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:182:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:189:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:206:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:427:5 • omit_local_variable_types
  lint • Prefer single quotes where they won't require escape sequences. • lib/src/base.dart:563:7 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • lib/src/base.dart:564:7 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • lib/src/base.dart:565:7 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • lib/src/base.dart:566:7 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • lib/src/base.dart:567:7 • prefer_single_quotes
  lint • Omit type annotations for local variables. • lib/src/base.dart:693:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/base.dart:694:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/io.dart:105:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/stack_trace.dart:34:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/stack_trace.dart:38:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/stack_trace.dart:39:8 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/stack_trace.dart:55:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • lib/src/utils.dart:30:3 • omit_local_variable_types
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:13:11 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:16:21 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:28:15 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:29:21 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:30:18 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:31:22 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:32:20 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:32:27 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:35:20 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/event_test.dart:36:51 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/sentry_browser_test.dart:4:9 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/sentry_io_test.dart:4:9 • prefer_single_quotes
  lint • Omit type annotations for local variables. • test/stack_trace_test.dart:12:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/stack_trace_test.dart:24:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/stack_trace_test.dart:84:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:26:3 • omit_local_variable_types
  lint • The function testCaptureException should have a return type but doesn't. • test/test_utils.dart:46:1 • always_declare_return_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:51:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:56:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:66:3 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:82:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:175:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:185:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:195:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:204:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:213:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:217:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:226:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:242:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:271:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:273:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:283:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:299:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:311:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/test_utils.dart:314:5 • omit_local_variable_types
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:329:13 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:330:19 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:331:16 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:332:20 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:334:13 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:335:19 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:336:16 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:337:20 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:338:18 • prefer_single_quotes
  lint • Prefer single quotes where they won't require escape sequences. • test/test_utils.dart:338:25 • prefer_single_quotes
  lint • Omit type annotations for local variables. • test/test_utils.dart:340:5 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/utils_test.dart:12:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/utils_test.dart:21:7 • omit_local_variable_types
  lint • Omit type annotations for local variables. • test/utils_test.dart:61:7 • omit_local_variable_types
75 lints found.

After the changes:

+ dartanalyzer --fatal-warnings ./
Analyzing sentry-flutter...
No issues found!

bruno-garcia
bruno-garcia commented Apr 11, 2020
View reviewed changes
Copy link
Member Author

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Commented on the reasons why I ignore some of the linter's suggestion.
I'd appreciate in some advise on the way to go there

lib/src/base.dart
@@ -891,10 +893,12 @@ class Runtime {
final String rawDescription;

const Runtime({this.key, this.name, this.version, this.rawDescription})
// ignore: prefer_is_empty
Copy link
Member Author

Choose a reason for hiding this comment

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

If the prefer_is_empty suggestion is taken here it fails with Invalid constant value.dart(invalid_constant).

Copy link
Member Author

@bruno-garcia bruno-garcia Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Actually this one is affecting the score on pub.dev:

image

Would be nice to find a solution. Or does this // ignore: affect the scoring?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a gap in the language. /cc @lrhn @leafpetersen @eernstg

Copy link

Choose a reason for hiding this comment

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

Sorry, .length is literally called out specially in the spec as one of the only things you're allowed to do in constants. Expanding the scope of what you can do in constants is a frequent request, so noted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, then it's a linter issue. The linter should not be in a conflict with the language. Which quickly led me to https://github.com/dart-lang/linter/issues/1719

@bruno-garcia
Copy link
Member Author

CI is green but doesn't update here, travis' web hooks probably broken.

@bruno-garcia bruno-garcia reopened this Apr 12, 2020
@bruno-garcia
Copy link
Member Author

@yjbanov Would you please code review? Thanks!

@bruno-garcia
Copy link
Member Author

Triggering the CI webhook again

@bruno-garcia bruno-garcia reopened this May 2, 2020
yjbanov
yjbanov approved these changes May 4, 2020
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Alternatively, we could pull some lints from analysis_options.dart in the flutter/flutter repo. This way this repo and flutter/flutter would use the same code style.

Leaving it to your taste.

@yjbanov yjbanov mentioned this pull request May 5, 2020
Closed
@bruno-garcia
Copy link
Member Author

I'll read up on the linting rules and file a PR. Thanks for the hint!

@yjbanov
Copy link
Contributor

yjbanov commented May 6, 2020

I'll read up on the linting rules and file a PR. Thanks for the hint!

@bruno-garcia How would you like to proceed w.r.t. which rules to apply? Do you want to submit this as is and update the linter rules in a separate PR? Or would you like to adopt Flutter's analysis options in this PR?

@bruno-garcia
Copy link
Member Author

@yjbanov if it's OK with you I'd merge this in to get rid of the warnings first.

We can discuss the linting rules to be brought in (I just scrolled over the rules file so far) on a separate PR?

yjbanov
yjbanov approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

LGTM

@yjbanov yjbanov merged commit f87f0f0 into getsentry:master May 11, 2020
@bruno-garcia bruno-garcia deleted the fix/linter-issues branch May 11, 2020 15:34
@bruno-garcia bruno-garcia mentioned this pull request Aug 6, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yjbanov yjbanov yjbanov approved these changes

@leafpetersen leafpetersen leafpetersen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bruno-garcia @yjbanov @leafpetersen