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.

Sign up for GitHub

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

Jump to bottom

[various] Add missing_code_block_language_in_doc_comment lint to flutter/packages. #6473

Merged
merged 17 commits into from
Nov 4, 2024
Merged

[various] Add missing_code_block_language_in_doc_comment lint to flutter/packages. #6473

merged 17 commits into from
Nov 4, 2024
+163 −108

Conversation

kallentu
Copy link
Contributor

@kallentu kallentu commented Apr 5, 2024 •
edited
Loading

Adds this Dartdoc-related lint to the flutter repository, in replacement of the Dartdoc warning (missingCodeBlockLanguage) because it will be deprecated and removed soon.

flutter/flutter already has this lint as well. Adding to flutter/engine with flutter/engine#51944.

Lint Proposal: dart-lang/linter#4904

Issue covering future work removing the // ignore:s: flutter/flutter#157620

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@kallentu kallentu changed the title Add missing_code_block_language_in_doc_comment lint to flutter/packages. Add missing_code_block_language_in_doc_comment lint to flutter/packages. Apr 5, 2024
@kallentu kallentu changed the title Add missing_code_block_language_in_doc_comment lint to flutter/packages. [various] Add missing_code_block_language_in_doc_comment lint to flutter/packages. Apr 5, 2024
@stuartmorgan stuartmorgan added the waiting for stable update Can't be landed until functionality reaches the stable channel label Apr 8, 2024
@stuartmorgan
Copy link
Contributor

Is this lint in 3.4, or do we need to wait another stable release cycle?

@Piinks
Copy link
Contributor

Piinks commented Oct 22, 2024

(PR triage): What is the status of this PR?

@kallentu
Copy link
Contributor Author

kallentu commented Oct 24, 2024 •
edited
Loading

(PR triage): What is the status of this PR?

@Piinks
Linux analyze legacy N-1 is failing because it's linting on the wrong things. There's a commit that just so happens to fall in 3.5 (dart-lang/sdk@b6ac281) which isn't picked up in that tryjob. The rest of the lint work is done in 3.4 and the version of legacy N-1 is at Dart 3.4.x.

What's the best way forward? I don't want to be doing the fix for what's failing because it's the wrong behaviour. I'm happy to wait longer, but I suspect N-1 will become N-2 and that will end up failing.

@Piinks
Copy link
Contributor

Piinks commented Oct 24, 2024

Hmm, I am not sure. @stuartmorgan would likely be best to advise.
I think you would have to bump the dependency on each affected package to make it no longer compatible with N-1 and N-2.

@kallentu
Copy link
Contributor Author

The options are:

  • Drop support for N-1 in those packages
  • Wait for the next stable to do this, and drop N-2 in those packages
  • Wait for two more stables (I would not recommend this one; the value of N-2 support is low)
  • Add // ignore annotations for the N-1 false positives, with a comment explaining why and linking to an issue that tracks removing them after the next stable update. There's no warning for unnecessary // ignores, so that will work on all versions.

Any of those is fine. Our policy is that we only commit to supporting N, so the bar for dropping N-1 is low; N-1/N-2 tests are to prevent accidentally doing something that doesn't compile in N-1/N-2 and shipping it in a version that claims to support that version, thus breaking clients.

Added ignores as you suggested with the issue tracking it: flutter/flutter#157620.

Fixed up pubspec and changelogs. Should be ready for review and to move forward.

stuartmorgan
stuartmorgan approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM with minor changes.

packages/flutter_migrate/CHANGELOG.md Outdated Show resolved Hide resolved
packages/process/CHANGELOG.md Show resolved Hide resolved
chunhtai
chunhtai approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

ditman
ditman approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM, but there's some TODOs that could be done now I think?

packages/rfw/lib/src/flutter/material_widgets.dart Show resolved Hide resolved
packages/webview_flutter/webview_flutter_web/lib/src/http_request_factory.dart Outdated Show resolved Hide resolved
packages/rfw/lib/src/dart/binary.dart Outdated Show resolved Hide resolved
@kallentu kallentu merged commit 79f8b0b into flutter:main Nov 4, 2024
76 checks passed
@kallentu kallentu deleted the code-block-lint branch November 4, 2024 22:24
@ditman
Copy link
Member

ditman commented Nov 4, 2024

Thanks @kallentu!!

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 5, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2024
@engine-flutter-autoroll
79f8b0b66 [various] Add missing_code_block_language_in_doc_comment …
dccaf99 
…lint to flutter/packages. (flutter/packages#6473)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 5, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 5, 2024
@engine-flutter-autoroll
79f8b0b66 [various] Add missing_code_block_language_in_doc_comment …
f9d34ac 
…lint to flutter/packages. (flutter/packages#6473)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Nov 5, 2024
@engine-flutter-autoroll
Roll Packages from 796afa35071d to 721943194945 (11 revisions) (#158179)
49a16fc 
flutter/packages@796afa3...7219431

2024-11-05 [email protected] [vector_graphics*] Relax dependency constraints of vector_graphics, vector_graphics_codec, vector_graphics_compiler, flutter_svg  (flutter/packages#8018)
2024-11-04 [email protected] [various] Add `missing_code_block_language_in_doc_comment` lint to flutter/packages. (flutter/packages#6473)
2024-11-04 [email protected] [various] Update example apps to Kotlin 1.9.0 (flutter/packages#7998)
2024-11-04 [email protected] [go_router] add current state getter (flutter/packages#7651)
2024-11-04 [email protected] Applied Gradle Plugins Declaratively for Multiple Plugin Example Apps (flutter/packages#7968)
2024-11-04 [email protected] Roll Flutter from f86b777 to 8591d0c (16 revisions) (flutter/packages#8015)
2024-11-04 [email protected] [camera_windows] Revert: Support image streams on Windows platform (flutter/packages#7951)
2024-11-02 [email protected] [camera] Use Pigeon for Windows C++->Dart (flutter/packages#8001)
2024-11-02 [email protected] [script/tool] update dependencies (flutter/packages#7992)
2024-11-01 [email protected] Roll Flutter from 0fe6153 to f86b777 (16 revisions) (flutter/packages#8000)
2024-11-01 [email protected] [path_parsing] deprecate utility functions that should be private (flutter/packages#7993)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chunhtai chunhtai chunhtai approved these changes

@ditman ditman ditman approved these changes

@cbracken cbracken cbracken approved these changes

@yjbanov yjbanov yjbanov approved these changes

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@domesticmouse domesticmouse domesticmouse approved these changes

@bparrishMines bparrishMines Awaiting requested review from bparrishMines bparrishMines is a code owner

@Hixie Hixie Awaiting requested review from Hixie Hixie is a code owner

Assignees
No one assigned
Labels
p: flutter_migrate p: go_router p: google_sign_in p: rfw Remote Flutter Widgets p: web_benchmarks p: webview_flutter platform-android platform-ios platform-web waiting for stable update Can't be landed until functionality reaches the stable channel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kallentu @stuartmorgan @Piinks @ditman @domesticmouse @yjbanov @cbracken @chunhtai