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

add several rules to core and recommended #150

Merged
merged 2 commits into from
Sep 20, 2023
Merged

add several rules to core and recommended #150

merged 2 commits into from
Sep 20, 2023

Conversation

devoncarew
Copy link
Member

  • add no_wildcard_variable_uses, secure_pubspec_urls, and type_literal_in_constant_pattern to core
  • add deprecated_consistency, unnecessary_to_list_in_spreads, and use_super_parameters to recommended
  • remove prefer_equal_for_default_values from recommended

Keeping the version at 3.0.0-wip.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@devoncarew devoncarew requested a review from pq September 19, 2023 21:47
@@ -21,6 +21,7 @@ linter:
- avoid_single_cascade_in_expression_statements
- constant_identifier_names
- control_flow_in_finally
- deprecated_consistency
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to double check the behavior around class/constructor deprecation?

Copy link
Member

Choose a reason for hiding this comment

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

I confirmed that the lint does currently ask for constructors to be deprecated when the class is deprecated - I think we had discussed changing that.

I do notice that this requirement does impact what deprecations you see on usage of a type alias - if a constructor is not deprecated then usage of that constructor through an alias is not flagged with a diagnostic.

@deprecated
class C {
  C();
  @deprecated
  C.namedDeprecated();
}

typedef Renamed = C;

void main() {
  Renamed(); // No diagnostic
  Renamed.namedDeprecated(); // Diagnostic

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, good catch. It looks like @pq has investigated this in dart-lang/linter#4752 and we're free to remove the requirement (from the lint) to deprecate both the class and the ctor.

Let's delay adding this lint to the set until we've shipped an SDK w/ that update for the lint; I'll remove from this PR and update the github project.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. This is interesting.

One thing to note is that you should see a diagnostic on the typedef declaration.

typedef Renamed = C; // Diagnostic

But you're right that you won't see it on Renamed().

I guess in practice if you exported the typedef maybe this could be a footgun for users but then you've made a conscious choice to ignore the diagnostic reported at the typedef declaration so maybe that's on you? Not sure what the right answer is... 🤔

@devoncarew devoncarew merged commit b044aca into main Sep 20, 2023
4 checks passed
@devoncarew devoncarew deleted the review_2 branch September 20, 2023 18:50
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

3 participants