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

refactor: Add new lint rules #2477

Merged
merged 9 commits into from
Apr 13, 2023
Merged

refactor: Add new lint rules #2477

merged 9 commits into from
Apr 13, 2023

Conversation

spydon
Copy link
Member

@spydon spydon commented Apr 10, 2023

Description

This PR adds the following lint rules to our list:

always_put_required_named_parameters_first
avoid_multiple_declarations_per_line
avoid_positional_boolean_parameters
avoid_returning_null_for_void
avoid_returning_this
avoid_unnecessary_containers
enable_null_safety
library_private_types_in_public_api
no_leading_underscores_for_library_prefixes
no_leading_underscores_for_local_identifiers
prefer_null_aware_method_calls
tighten_type_of_initializing_formals
unnecessary_late
use_setters_to_change_properties

And these rules were considered, and some changes were made according to them as a clean-up, but in many places they didn't make sense (prefer_asserts_with_message I would have included, but there were too many places that needed to be changes):

collection_methods_unrelated_type
prefer_asserts_with_message
avoid_renaming_method_parameters

Checklist

  • I have followed the [Contributor Guide] when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • No, this PR is not a breaking change.

if (keepOldPosition) {
_oldPositionByItem.remove(node);
_oldPositionByItem.remove(hitbox);
Copy link
Member Author

Choose a reason for hiding this comment

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

Does this change make sense @ASGAlex? And also, shouldn't keepOldPosition be inverted in the if?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right the both things are bugs.
But I see that function that uses it - hasMoved - is not used anywhere, just mentioned in comments.
Looks like I thought about future optimization here but idea was lost then...
So we have two options here:

  1. Fixing mentioned bugs
  2. Safely remove unsed functionality.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, sounds like the best option is removing it then.
Would you like to make a PR for removing it? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ASGAlex Hmm, but how can it work if hasMoved isn't used? Shouldn't that be used to determine if new quadrants might need to be created, or does it always check that now?

@spydon spydon requested review from st-pasha and a team April 10, 2023 17:27
@spydon spydon requested a review from a team April 10, 2023 17:32
Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

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

LOVE IT!

@spydon spydon enabled auto-merge (squash) April 13, 2023 19:22
@spydon spydon merged commit dbda37b into main Apr 13, 2023
@spydon spydon deleted the spydon/new-lint-rules branch April 13, 2023 19:42
@Gustl22
Copy link
Contributor

Gustl22 commented Apr 18, 2023

@spydon Isn't this change breaking in the sense, that linter (flutter analyze) fails for projects, which do upgrade minor versions / patches automatically (like audioplayers)? I'd recommend to release a major version for flame_lint :)

@spydon
Copy link
Member Author

spydon commented Apr 18, 2023

@spydon Isn't this change breaking in the sense, that linter (flutter analyze) fails for projects, which do upgrade minor versions / patches automatically (like audioplayers)? I'd recommend to release a major version for flame_lint :)

For semver, when versions are less than 1.0.0, the users can expect breaking changes. You're right that it is breaking though and should have been marked as such.

@Gustl22
Copy link
Contributor

Gustl22 commented Apr 18, 2023

Alright then :)

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.

None yet

8 participants