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

feat(neon_lints): upgrade lints #2707

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Leptopoda
Copy link
Member

@Leptopoda Leptopoda commented Dec 12, 2024

This might need some discussion.

Why are some rules disabled? lint_maker now skipps experimental lints.
TBH this was an oversight on my side as I didn't think about neon when making the change.

Do we want to enable experimental rules?


use_super_parameters for example, is experimental but also recommended. We didn't ever encounter any issues with this rule, and it has a quick fix available. unsafe_html raised some false positives so I'd like to not enable this one.

https://dart.dev/tools/linter-rules/use_super_parameters

Signed-off-by: Nikolas Rimikis <[email protected]>
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 28.90%. Comparing base (5b430b4) to head (3511709).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2707   +/-   ##
=======================================
  Coverage   28.90%   28.90%           
=======================================
  Files         373      373           
  Lines      136645   136645           
=======================================
  Hits        39498    39498           
  Misses      97147    97147           
Flag Coverage Δ *Carryforward flag
account_repository 98.47% <ø> (ø)
cookie_store 99.48% <ø> (ø)
dashboard_app 96.05% <ø> (ø)
dynamite 31.05% <ø> (ø)
dynamite_end_to_end_test 61.69% <ø> (ø)
dynamite_runtime 85.40% <ø> (ø)
interceptor_http_client 97.18% <ø> (ø)
neon_dashboard 96.05% <ø> (ø) Carriedforward from 5b430b4
neon_framework 59.34% <ø> (ø)
neon_http_client 94.32% <ø> (ø)
neon_notifications 100.00% <ø> (ø) Carriedforward from 5b430b4
neon_rich_text 100.00% <ø> (ø)
neon_storage 94.66% <ø> (ø)
neon_talk 99.45% <ø> (ø) Carriedforward from 5b430b4
nextcloud 24.33% <ø> (ø)
notifications_app 97.36% <ø> (ø)
notifications_push_repository 98.11% <ø> (ø)
sort_box 90.90% <ø> (ø)
talk_app 98.83% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Everything rule I didn't comment on we can remove (for various reasons)

use_test_throws_matchers: true
use_to_and_as_if_applicable: true
valid_regexps: true
void_checks: true
avoid_as: false
no_default_cases: false
Copy link
Member

Choose a reason for hiding this comment

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

I like this one, can we enable it?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I just saw we already had it above, but also disabled 🤔

use_named_constants: true
use_raw_strings: true
use_rethrow_when_possible: true
use_setters_to_change_properties: true
use_string_buffers: true
use_string_in_part_of_directives: true
use_super_parameters: true
Copy link
Member

Choose a reason for hiding this comment

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

This was already helpful in the past, so I'd like to keep it enabled

@@ -219,16 +211,15 @@ linter:
use_if_null_to_convert_nulls_to_bools: true
use_is_even_rather_than_modulo: true
use_key_in_widget_constructors: true
use_late_for_private_fields_and_variables: true
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense to me, can we keep it?

@@ -209,7 +202,6 @@ linter:
unnecessary_to_list_in_spreads: true
unreachable_from_main: true
unrelated_type_equality_checks: true
unsafe_html: true
Copy link
Member

Choose a reason for hiding this comment

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

I did fix these issues, so we should be able to keep this rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is actually deprecated and removed in dart 3.7

https://dart.dev/tools/linter-rules/unsafe_html

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, it was only added so recently 😅

@@ -196,7 +190,6 @@ linter:
unnecessary_new: true
unnecessary_null_aware_assignments: true
unnecessary_null_aware_operator_on_extension_on_nullable: true
unnecessary_null_checks: true
Copy link
Member

Choose a reason for hiding this comment

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

Also makes sense to me, can we keep it?

@@ -14,7 +14,6 @@ linter:
always_specify_types: false
always_use_package_imports: true
annotate_overrides: true
annotate_redeclares: true
Copy link
Member

Choose a reason for hiding this comment

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

I think this is important for extension types?

exhaustive_cases: true
file_names: true
flutter_style_todos: true
hash_and_equals: true
implementation_imports: true
implicit_call_tearoffs: true
implicit_reopen: true
Copy link
Member

Choose a reason for hiding this comment

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

Seems important for the new class modifiers

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