Skip to content

[pigeon] reorg generator files #8532

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

Merged
merged 7 commits into from
Feb 3, 2025
Merged

Conversation

tarrinneal
Copy link
Contributor

Just moves a few files into folders to clean things up a bit. Future organizing is likely, but I wanted to move the files with no changes first

@tarrinneal
Copy link
Contributor Author

And I do believe this is exempt from versioning

@stuartmorgan-g
Copy link
Contributor

This is actually a breaking change. The Dart package convention is that anything in lib/src/ is private to the package and should never be imported directly by clients, but anything else in lib/ is fair game, so currently it would be fine, by language conventions, for someone to write code that imports these Pigeon files directly.

If we don't want the generator files to be directly imported (which I think we don't), we should do a breaking change to move a bunch of things into src/, and then in the future we can reorganize src/ however we want.

@tarrinneal
Copy link
Contributor Author

I bumped the version and moved everything into src. I can wait to do this along with another breaking change if you'd like, but I think editing the files and moving them is a bit messy for reviews.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g 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 changelog nit.

I can wait to do this along with another breaking change if you'd like, but I think editing the files and moving them is a bit messy for reviews.

I expect ~0 people to actually be broken by this change, and we've always treated major version changes for pigeon as low-cost, so I don't think there's any real value in combining (and as you point out, not-trivial downsides for our ability to do code reviews).

@@ -4,7 +4,7 @@

import 'dart:io' show exit;

import 'package:pigeon/pigeon_cl.dart';
import 'package:pigeon/src/pigeon_cl.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I guess bin/ has special-case allowance in the analyzer to import from the same package's src even if it's using a package: import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was surprised it was allowed as well, since the pigeon_cl.dart file is used outside of lib and tests, should I move it back out of src?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I found a reference later on the page I had linked to, it's explicitly allowed:

You are free to import libraries that live in lib/src from within other Dart code in the same package (like other libraries in lib, scripts in bin, and tests) but you should never import from another package's lib/src directory.

And that's the preferred format for such an include:

How you import libraries from within your own package depends on the locations of the libraries:

When reaching inside or outside lib/ (lint: avoid_relative_lib_imports), use package:.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

I didn't push the button again. I need a Chrome extension for this.

@tarrinneal
Copy link
Contributor Author

We should land #8302 first to avoid a ton of churn on that pr

@tarrinneal tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 3, 2025
@auto-submit auto-submit bot merged commit 9c9006f into flutter:main Feb 3, 2025
78 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 5, 2025
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Feb 5, 2025
flutter/packages@02c6fef...e6ce02c

2025-02-05 [email protected] [vector_graphics]
Allow transition between placeholder and loaded image to have an
animation (flutter/packages#8195)
2025-02-04 [email protected] [flutter_markdown] Make custom table
column alignments work when text wraps (flutter/packages#8340)
2025-02-04 [email protected]
[interactive_media_ads] Adds internal wrapper for iOS native
`IMAAdPodInfo` (flutter/packages#8429)
2025-02-03 [email protected] [pigeon] reorg generator files
(flutter/packages#8532)
2025-02-03 [email protected] [pigeon] [swift] Fix `PigeonError`
sendability conformance in Swift 6 (flutter/packages#8302)
2025-02-03 [email protected] Roll Flutter from
b007899 to 8e2a6fc (61 revisions) (flutter/packages#8556)
2025-02-03 [email protected]
[google_maps_flutter] Support for Ground Overlay - platform interface
(flutter/packages#8518)
2025-01-31 [email protected] [tool] Add
--xcode-warnings-exceptions flag (flutter/packages#8524)
2025-01-31 [email protected] [tool] Ensure that injected
dependency overrides are sorted (flutter/packages#8542)
2025-01-31 [email protected] [vector_graphics] Revert leak
tracker change (flutter/packages#8544)
2025-01-31 [email protected] [shared_preferences_tool] Loosen
vm_service constraint to allow for 15 (flutter/packages#8539)
2025-01-31 [email protected]
[in_app_purchase] Activate leak testing for android
(flutter/packages#8369)
2025-01-31 [email protected] [flutter_markdown] Allow tables to be
scrollable with IntrinsicColumnWidth (flutter/packages#8526)
2025-01-30 [email protected] Update CODEOWNERS for pkg:animations
(flutter/packages#8534)
2025-01-30 [email protected] Roll Flutter from
c1ffaa9 to b007899 (43 revisions) (flutter/packages#8527)
2025-01-30 [email protected] [video_player_web] Adjust Web
implementation to the new platform interface (flutter/packages#8528)
2025-01-30 [email protected] [shared_preferences] Exposed
SharedPreferencesOptions. (flutter/packages#8530)
2025-01-29 [email protected] Re-land [shared_preferences] Add
shared preferences devtool (flutter/packages#8531)
2025-01-29 [email protected]
[in_app_purchase_storekit] Add Swift Package Manager compatibility
(flutter/packages#8469)
2025-01-29 [email protected] Revert "Re-land [shared_preferences]
Add shared preferences devtool" (flutter/packages#8529)
2025-01-29 [email protected]
[go_router_builder] Fixes trailing `?` by comparing iterables
(flutter/packages#8521)
2025-01-29 [email protected] [tool] Refactor
args of strings or YAML file lists (flutter/packages#8513)
2025-01-28 [email protected] [go_router]
Add missing await keyword to onTap callback in the code example in
`navigation.md` (flutter/packages#8343)
2025-01-28 [email protected] Re-land [shared_preferences] Add
shared preferences devtool (flutter/packages#8519)
2025-01-28 [email protected]
[vector_graphics] Fix memory leaks and activate leak testing
[prod-leak-fix] (flutter/packages#8373)

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
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: pigeon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants