Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

matanlurey
Copy link
Contributor

@matanlurey matanlurey commented May 15, 2024

Based on the (internal) discussion around converging on using the official Dart style guide, with the exception of the code that gets published under dart:ui, as that is user-facing, and we'd like to evolve the code style in conjunction with the framework.

I also took the opportunity to specify more about our style guide use in general, mostly to make it easier to understand our conventions, and also call out known problem areas (notably, our over-use of shared_ptr and auto in some cases). I am happy to split those up, but it was easier to make the markdown changes at once.

I also took @cbracken and folks advice and clarified directly that explicit types in Dart are not bad (with examples).

- always_put_control_body_on_new_line
# - always_put_required_named_parameters_first # we prefer having parameters in the same order as fields https://github.com/flutter/flutter/issues/10219
- always_specify_types
# - always_specify_types # no longer used; see https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#dart
Copy link
Member

Choose a reason for hiding this comment

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

Please mark this as described at the top of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks for pointing that out.

Comment on lines +77 to +79
The Flutter engine _intends_ to follow the [Dart style guide][dart_style] but
currently follows the [Flutter style guide][flutter_style], with the following
exceptions:
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to learn more about why it was decided to make that shift and how it'll benefit us to have diverging style guides in the engine and framework. Where can I learn more?

Copy link
Contributor

Choose a reason for hiding this comment

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

The root problem is that we already have diverging style guides: Dart vs. Flutter.

As a project, Dash needs to settle on a single style guide (and automatic formatter) for every language we use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I talked to Michael a bit 1:1, but John already answered the question authoritatively. I also tried to explain our reasoning as well as possible in this file, and I'm open to any specific questions on the examples provided below.

Hixie
Hixie previously requested changes May 15, 2024
Copy link
Contributor

@Hixie Hixie left a comment

Choose a reason for hiding this comment

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

Before making this kind of dramatic change we should have a proper discussion with a design doc and Dash Forum discussion and so on. There's lots of tradeoffs to consider.

@johnmccutchan
Copy link
Contributor

johnmccutchan commented May 15, 2024

The folks who work day-to-day in the engine repository have decided that they want to remove always_specify_types for two reasons:

  1. They feel that the ergonomics will be improved for Dart code that they write in the engine repository.
  2. We have a larger plan to standardize the Dash project on one style per language and disabling this lint is inline with adopting Dart's style guide.

I (and I imagine the rest of the engine team) am interested in hearing any feedback from people who regularly contribute to the engine repository.

Feedback from other people is still appreciated, but, it will be scaled according to how often they work in this repository.

@matanlurey
Copy link
Contributor Author

matanlurey commented May 15, 2024

Fwiw, I'd be happy to keep the style guide in place on dart:ui (lib/ui) for the time being (but agree with John).

The vast majority of Dart code in the engine is custom tooling that is not exposed to end-users at all.

@Hixie
Copy link
Contributor

Hixie commented May 15, 2024

I think it's very important that we take into account the needs of our team, certainly, including especially folks directly working on the relevant code. It's not the only input, however, and as the project's tech lead I am requesting that you not unilaterally change long-established project-wide policies without presenting the needs that are leading to those desired changes, objectively considering the needs of our users, and generally working in the open way that Flutter's core values are all about.

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

I'm in favor of being in alignment with the official style guides, despite my personal feelings about what is best. The Dart recommendations sound good to me.

The C++ section is a bit confusing for me. Are you explaining that we deviate from the style guide but we shouldn't be? It doesn't deviate from the C++ style guide so it could be left unsaid imo. It kind of confuses wether these are exceptions or just being called out as mea culpas.

@cbracken
Copy link
Member

cbracken commented May 15, 2024

To be clear, my personal position is that I strongly prefer that the type of a given expression be obvious. I find it difficult to read and review code where the types are unobvious; types aren't just for the compiler -- they're very useful to readers in helping them reason about the code.

Forcing explicitly-typed declarations is one approach to making the types obvious, but I agree it often does more harm than good to readability. As an example, in platform_isolate.dart we have this gem:

final Map<int, Completer<Object?>> _pending = <int, Completer<Object?>>{};

which seems far less readable than:

final _pending = <int, Completer<Object?>>{};

Taking it a step further, the final line seems obvious enough to not need a type at all:

final logBuffer = StringBuffer();
// ... add messages
final log = logBuffer.toString();

Overall, I'm in favour of having the style guide include explicit guidance that the types be obvious to the reader, but leaving the actual implementation of that obviousness to a discussion between the author and the reviewer rather than enforcing brute-force via tooling.

TL;DR with the changes I suggested above, this approach would lgtm.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Post-update, the issues I had are all addressed.

@matanlurey matanlurey dismissed Hixie’s stale review May 16, 2024 01:04

We aren't considering a design forum for this change, similar to the lack of design forum for other analysis options changes.

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2024
@auto-submit auto-submit bot merged commit e6e37b4 into flutter:main May 16, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2024
@zanderso
Copy link
Member

Reason for revert: This sort of change is in scope for a discussion at the Dash forum.

@zanderso zanderso added the revert Label used to revert changes in a closed and merged pull request. label May 16, 2024
auto-submit bot pushed a commit that referenced this pull request May 16, 2024
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label May 16, 2024
auto-submit bot added a commit that referenced this pull request May 16, 2024
#52859)" (#52867)

Reverts: #52859
Initiated by: zanderso
Reason for reverting: This sort of change *is* in scope for a discussion at the Dash forum.
Original PR Author: matanlurey

Reviewed By: {cbracken, gaaclarke, johnmccutchan}

This change reverts the following previous change:
Based on the (internal) discussion around converging on using the official Dart style guide, with the exception of the code that gets published under `dart:ui`, as that is user-facing, and we'd like to evolve the code style in conjunction with the framework.

I also took the opportunity to specify more about our style guide use in general, mostly to make it easier to understand our conventions, and also call out known problem areas (notably, our over-use of `shared_ptr` and `auto` in some cases). I am happy to split those up, but it was easier to make the markdown changes at once.

I also took @cbracken and folks advice and clarified directly that explicit types in Dart are _not_ bad (with examples).
@matanlurey matanlurey deleted the flutter-engine-style-types branch May 16, 2024 03:21
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 16, 2024
…148448)

flutter/engine@c11d64b...65ac4bf

2024-05-16 [email protected] Include `stdout` on a failed `gn desc` call, and test for it. (flutter/engine#52863)
2024-05-16 [email protected] Revamp the engine style guide, remove `always_specify_types`. (flutter/engine#52859)
2024-05-16 [email protected] Roll Skia from bbe06eb4a8e9 to 91527e447923 (1 revision) (flutter/engine#52864)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@Hixie
Copy link
Contributor

Hixie commented May 16, 2024

Landing a change while there is an outstanding review comment is a violation of our policies and extremely rude, which also makes it a violation of our code of conduct.

matanlurey added a commit to matanlurey/engine that referenced this pull request Jun 5, 2024
auto-submit bot pushed a commit that referenced this pull request Jun 5, 2024
…_types` (#53223)

Based on the (internal) discussion around converging on using the official Dart style guide, the design discussion https://flutter.dev/go/use-dart-style-in-flutter-engine, and with the exception of the code that gets published under `dart:ui` (as that is user-facing, and we'd like to evolve the code style in conjunction with the framework), we're going to be (gradually) adopting the Dart style guide in `flutter/engine`.

For now, we:
- Remove `always_specify_types` (except for `lib/ui`, i.e. `dart:ui`)
- Re-enable `type_annotate_public_apis` (which is now relevant since the above is disabled)
- Announce our _intent_ to re-format and apply the Dart formatter (which we'll land in the near future)

---

I also took the opportunity to specify more about our style guide use in general, mostly to make it easier to understand our conventions, and also call out known problem areas (notably, our over-use of `shared_ptr` and `auto` in some cases). I am happy to split those up, but it was easier to make the markdown changes at once.

I also took @cbracken and folks advice and clarified directly that explicit types in Dart are _not_ bad (with examples).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants