leancodepl / flutter_corelibrary Goto Github PK

dart-lang/linter has just got v1.24.0.

TLDR:

New lints:

• unnecessary_to_list_in_spreads
• unnecessary_null_aware_operator_on_extension_on_nullable

Updated lints:

• use_colored_box to not flag nullable colors

When using internalization (e.g. through a package such as i18n), no text placed in the UI should be hardcoded to one language. We could implement a custom linter rule to prevent this from happening.

@gogolon Let's do the research about similar packages, e.g. infospect, alice, logman and then decide together what to do with this package.

Dart 2.16 introduces a few new lints. We should consider adding some/all of them to leancode_lint.

@shilangyu @RobertOdrowazLNCD @Albert221

#12 deprecates the credentialsChangedCallback LoginClient constructor parameter.

Remove it before the next major version (v2.0.0).

Usage section is obsolete. Currently more named params are required.

Suggested example of usage:

final apiEndpoint = await overrideApiEndpoint(
  sharedPreferences: await SharedPreferences.getInstance(),
  getInitialUri: getInitialUri,
  deeplinkOverrideSegment: 'override',
  deeplinkQueryParameter: 'apiAddress',
  defaultEndpoint: Uri.parse('https://api.test.lncd.pl'),
);

• new use_super_parameters
• new use_enums
• new use_colored_box
• marked stable null_check_on_nullable_type_parameter

Currently all our packages are using lint.
Do we want to replace it with leancode_lint?

Currently there is only a method onCredentialsChanged that gives you new Credentials? object you base .

I have a case where I'd need read to access the accessToken and perform some action outside LoginClient with it. Currently there is no way to do so.

In Patrol, I have accumulated many TODO comments, and I'm not happy with their quality. I would like to be able enforce "good TODOs" - TODOs that have a link to a GitHub issue so the team doesn't lose track of them.

Examples

BAD

// TODO: set reasonable duration or create new exception for this case

// TODO(bartekpacia): set reasonable duration or create new exception for this case

// TODO(bartekpacia): set reasonable duration or create new exception for this case. See #2137

The last is wrong because it makes no sense to include username in the TODO since the person responsible for the issue will very likely be the author of the issue.

GOOD

// TODO: set reasonable duration or create new exception for this case. See https://github.com/leancodepl/patrol/issues/2137

or we could go full-minimalism style - just require the issue number:

// TODO: set reasonable duration or create new exception for this case. See #2137

To paraphrase:

• GOAL: Enforce every TODO comment to have See #<github-issue-number at the end
• GOAL: Enforce every TODO comment to not have username (e.g. `// TODO(bartekpacia): ...)
• NON GOAL: Check if the provided issue link/number is valid/exists/is really an issue (and not a PR or a discussion)

Question

I'm not sure how multiline TODOs should be treated. Should it be:

// TODO(bartekpacia): set reasonable duration or create new exception for this case
// See https://github.com/leancodepl/patrol/issues/2137

or:

// TODO(bartekpacia): set reasonable duration or create new exception for this case
// TODO(bartekpacia): See https://github.com/leancodepl/patrol/issues/2137

?

There's a newer melos version that isn't backwards compatible and requires changes to melos.yaml + adding workspace-level pubspec. I'm not sure how many people still use melos 2 but it'd be nice to not have to swap versions for those who already have v3 installed

https://melos.invertase.dev/guides/migrations#200-to-300

Examples of code that is invalid when the discarded_futures lint is on:

Example 1

bad

TextButton(
  onPressed: () => Navigator.of(context).push(
    MaterialPageRoute<void>(
      builder: (_) => const WebViewScreen(),
    ),
  ),
  child: const Text('Open webview screen'),
),

good
Gotta add the async.

TextButton(
  onPressed: () async => Navigator.of(context).push(
    MaterialPageRoute<void>(
      builder: (_) => const WebViewScreen(),
    ),
  ),
  child: const Text('Open webview screen'),
),

Example 2

This is worse than the first example.

Let's say the we have a method with the following signature in our widget: Future<Position> _determinePosition() async.

Then this code is invalid:

FutureBuilder<Position>(
  future: _determinePosition(),
  builder: (context, snapshot) {
    // ...
  }
)

In this case, I worked around by making the method into a getter, but it wouldn't be possible if I needed to pass some arguments.

Example 3

Technically it's correct, but marking the closure as async messes with mocktail and tests fail.

Do we want to keep this lint, or should we release leancode_lint v1.3.1 with this lint disabled (until it gets better?).

oauth2 package is being considered for deprecation and discontinuation.
Ideally, we should know our options of whether and how this affects us, the possible threats of using the no-longer maintained oauth2 package, and how do we want to address that.

Idea: I'd like to have an easy way to diff the rules enabled by flutter_lints and by leancode_lint.

Originally posted by @bartekpacia in #101 (review)

I think the name of the use_design_system_item lint rule is too specific for what it is actually capable of. It can be used for classes that are not related to design system (though I agree it's the most common use case, and the migration cost probably outweights the (lack of) benefits).

• new: discarded_futures
• new: unnecessary_to_list_in_spreads
• update: use_colored_box does not flag nullable colors
• new: unnecessary_null_aware_operator_on_extension_on_nullable

Dart 2.18 changelogs

Let's rename this package since it assumes LeanCode backend API

As of right now, the dependency conflicts with other linters like riverpod and solid_lints that uses ^0.6.0

flutter_lints v2 enabled use_build_context_synchronously, which we don't want, AFAIK.

leancode_lint v1.2.0 bumped flutter_lints to v2, so now it's included in leancode_lint :|

The same goes for library_private_types_in_public_api

Currently there is no way to use login_client_flutter with login_client 3.0.0 because of the dependency login_client: ^2.0.1. We should publish login_client_flutter 3.0.0 with the dependency bumped.

When using leancode_lint on Flutter beta, it spits out:

another example:

The cause lies here:

analyzer:
  strong-mode:
    implicit-casts: false
    implicit-dynamic: false

We should change it to:

analyzer:
  language:
    strict-casts: true
    strict-inference: true
    strict-raw-types: true

as suggested by Customizing static analysis docs.

Learn more:

• https://dart.dev/guides/language/analysis-options#enabling-additional-type-checks
• dart-lang/sdk#50680
• dart-lang/sdk#50679

cc: @Albert221 @shilangyu

https://pub.dev/packages/json_view

Just recently, there was some update to VSCode's YAML linting and now it hints if we do some mistake in workflow files.

Example:

It caught my attention, so I checked on GitHub docs and indeed, there's no ignore-tags, there's tags-ignore.

This means that our workflows do a bit more work than necessary. We should fix it.

See leancodepl/corelibrary#188 and leancodepl/corelibrary#301.

TL;DR: Operations combine Query and Command into one.

Implementation wise, as far as I know the prototype should be identical to Query. It differs only semantically for us.

Tagging codeowner: @Albert221, if out of time, I am willing to implement this.

Consider changing login_client api so that it clearly indicates that method returns Future<T>. For example _loginClient.initialize() requires await so it can be called initializeAsync instead.

• new: implicit_reopen
• new: unnecessary_breaks
• new: type_literal_in_constant_pattern
• new: invalid_case_patterns
• new: deprecated_member_use_from_same_package
• removed: enable_null_safety
• updated (to track static elements): unreachable_from_main

The initial idea was to base our lints on flutter_lints to be compatible with community standards. But this has long stopped being the case. We now patch flutter_lints instead of extending it. For example we disable the use_build_context_synchronously rule since we deemed it too restrictive.

I suggest we drop flutter_lints completely and just maintain our own set of rules. One less dependency also means we don't have to be wary of upgrading flutter_lints in fear that it might introduce/disable some unwanted/wanted rules.

• New: annotate_redeclares
• Marked stable: use_build_context_synchronously

Maybe it is worth revisiting use_build_context_synchronously?

Practical explanation

This screenshot shows 2 warnings.

diagnostics

Clicking on unused_element redirects us to dart.dev/tools, to a section about a particular diagnostic (aka diagnostic message – in our case the diagnostic is unused_element). This website doesn't use the "lint" word anywhere.

Diagnostics are specified in analysis_options.yaml like this:

analyzer:
  errors:
    unused_element: false
    unused_field: true

Learn more about diagnostic messages on dart.dev

lints

Clicking on use_build_context_synchronously redirects us to dart-lang.github.io/linter, to a page about a particular lint rule (aka lint).

Lints are specified in analysis_options.yaml like this:

linter:
  rules:
    use_build_context_synchronously: false
    library_private_types_in_public_api: true

Learn more about linter rules on dart.dev

output

I've talked with @RobertOdrowazLNCD about this today. We agreed that the distinction between diagnostics and lints is blurry.

He suggested that if possible, we should define both diagnostics and lints in analyzer, so there'll be a smaller chance of making a mistake and defining a diagnostic in linter.rules). i tried and looks like it's not possible (?)

PS I've found explanation of difference between diagnostics and lints here

• new: collection_methods_unrelated_type
• new: combinators_ordering
• new: dangling_library_doc_comments
• new: enable_null_safety
• new: implicit_call_tearoffs
• new: library_annotations
• new: unnecessary_library_directive
• new: unreachable_from_main
• new: use_string_in_part_of_directives
• changed: use_colored_box and use_decorated_box to not over-report on containers without a child.

Is this:

usePostFrameEffect(() {
  callback();
});

much better than the vanilla:

WidgetsBinding.instance.addPostFrameCallback((_) {
  someCallback();
);

?

The one (and only?) advantage that I can think of is the possibility to invoke callback conditionally by passing keys to usePostFrameEffect. I haven't ever encountered a use case for this tho.

We could make debug page button draggable, so it's easy to change its position if it covers some interactive controls of the app.

We could use this: https://pub.dev/packages/draggable_fab

In dart, the standard is to not write acronyms in uppercase unless it is only up to 2 letters long (see here: https://dart.dev/guides/language/effective-dart/style#do-capitalize-acronyms-and-abbreviations-longer-than-two-letters-like-words)

Is that something we want to do?

Tagging codeowner: @Albert221, if out of time, I am willing to implement this.

          Do we want to have a flat structure of text spans instead of using nested spans?

Originally posted by @Albert221 in #249 (comment)

As the package got more mature it's a good moment to enhance its README with a bit more advanced features.

Any feedback is welcome.

The current proposal for enhancements:

• Description of Query, Command, and Operation CQRS methods, abstractly.
  • Describing ValidationErrors
• Note showing how to create a query/command
  • Note shortly introducing to the Dart contracts generator.
    Yes, the package isn't tied with the generator anyhow, but in real life, those two are at least almost always used together and it'd be a good idea to explain the concept with real use-case tips.
• Maybe a simple graphical (i.e. a logo) for the package?

The following useState is marked:

class W extends HookWidget {
  const W({super.key});

  @override
  Widget build(BuildContext context) {
    if (Random().nextBool()) {
      return HookBuilder(
        builder: (context) {
          useState(1);
          return const SizedBox();
        },
      );
    }

    return const SizedBox();
  }
}

It shouldn't as it exists in a different hook scope. It happens because when analysing W it looks for all used hooks inside of it. Instead it should stop recursing down once it encounters a new hook scope. This is done in helpers.dart@getAllInnerHookExpressions.

raised here

@shilangyu @Albert221 @RobertOdrowazLNCD

As well as extending HookConsumerWidget.

Right now it's quite hard to manage the circular dependency on some kind of authentication (say) cubit and the login client itself. The authentication thing needs the login client, but the login client needs to somehow "push" the credentials changes back to this authentication thing. Streams were initially used in the leancode_login_client and they served a good purpose for that.

Right now it uses a simple callback because the OAuth client used it too, but for this purpose, a stream looks like a much more viable solution.

These have been completely removed from corelib and are not used at all by the contracts generator. At this point they are just legacy leftovers. Other than compatibility, they provide no value (they are just aliases for Query and Command).

Tagging codeowner: @Albert221, if out of time, I am willing to implement this.

Makes it impossible to log in again without restarting the app.

flutter:
flutter: NoSuchMethodError: The method 'openUrl' was called on null.
flutter: Receiver: null
flutter: Tried calling: openUrl("POST", Instance of '_SimpleUri')
flutter: #0      Object.noSuchMethod (dart:core-patch/object_patch.dart:51:5)
flutter: #1      IOClient.send
package:http/src/io_client.dart:31
flutter: #2      BaseClient._sendUnstreamed
package:http/src/base_client.dart:91
flutter: #3      BaseClient.post
package:http/src/base_client.dart:32
flutter: #4      resourceOwnerPasswordGrant
package:oauth2/src/resource_owner_password_grant.dart:83
flutter: #5      ResourceOwnerPasswordStrategy.execute
package:login_client/…/strategies/resource_owner_password_strategy.dart:44
flutter: #6      LoginClient.logIn
package:login_client/src/login_client.dart:127
flutter: #7      SignInFormCubit.submit
package:truffleboard/…/bloc/sign_in_form_cubit.dart:34
flutter: #8      SignInScreen._onSignIn
package:truffleboard/…/auth/sign_in_screen.dart:77
flutter: #9      SignInScreen.build.<anonymous closure>.<anonymous closure>
package:truffleboard/…/auth/sign_in_screen.dart:196
flutter: #10     _InkResponseState._handleTap
package:flutter/…/material/ink_well.dart:993
flutter: #11     _InkResponseState.build.<anonymous closure>
package:flutter/…/material/ink_well.dart:1111
flutter: #12     GestureRecognizer.invokeCallback
package:flutter/…/gestures/recognizer.dart:183
flutter: #13     TapGestureRecognizer.handleTapUp
package:flutter/…/gestures/tap.dart:598
flutter: #14     BaseTapGestureRecognizer._checkUp
package:flutter/…/gestures/tap.dart:287
flutter: #15     BaseTapGestureRecognizer.handlePrimaryPointer
package:flutter/…/gestures/tap.dart:222
flutter: #16     PrimaryPointerGestureRecognizer.handleEvent
package:flutter/…/gestures/recognizer.dart:476
flutter: #17     PointerRouter._dispatch
package:flutter/…/gestures/pointer_router.dart:77
flutter: #18     PointerRouter._dispatchEventToRoutes.<anonymous closure>
package:flutter/…/gestures/pointer_router.dart:122
flutter: #19     _LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:377:8)
flutter: #20     PointerRouter._dispatchEventToRoutes
package:flutter/…/gestures/pointer_router.dart:120
flutter: #21     PointerRouter.route
package:flutter/…/gestures/pointer_router.dart:106
flutter: #22     GestureBinding.handleEvent
package:flutter/…/gestures/binding.dart:358
flutter: #23     GestureBinding.dispatchEvent
package:flutter/…/gestures/binding.dart:338
flutter: #24     RendererBinding.dispatchEvent
package:flutter/…/rendering/binding.dart:267
flutter: #25     GestureBinding._handlePointerEvent
package:flutter/…/gestures/binding.dart:295
flutter: #26     GestureBinding._flushPointerEventQueue
package:flutter/…/gestures/binding.dart:240
flutter: #27     GestureBinding._handlePointerDataPacket
package:flutter/…/gestures/binding.dart:213
flutter: #28     _rootRunUnary (dart:async/zone.dart:1206:13)
flutter: #29     _CustomZone.runUnary (dart:async/zone.dart:1100:19)
flutter: #30     _CustomZone.runUnaryGuarded (dart:async/zone.dart:1005:7)
flutter: #31     _invoke1 (dart:ui/hooks.dart:265:10)
flutter: #32     _dispatchPointerDataPacket (dart:ui/hooks.dart:174:5)

We had a nice discussion here, but now it disappeared.

The nice summary is in this comment. Here's also the Slack thread (LeanCode members only).

I'm creating this issue so that topic won't be forgotten.

todo: leancode_lint does not use mobile_tools to publish to the pub, so it does not create appropriate tags in the repo

Originally posted by @Albert221 on Slack

Describe what each rule does, known shortcomings, and configuration options.

This lint is deprecated.

See also:

• dart-lang/linter#811

Create the CredentialsStorage implementation for:

• the Web platform that doesn't have a dependency on the dart:io or iOS/Android platforms and uses localStorage as its backend
• both mobile (iOS/Android) and Web platforms that's just an umbrella for the credentials storage above and the FlutterSecureCredentialsStorage. Probably involving Dart's conditional importing.

/cc @mchudy @floweralex