Skip to content

[ffigen] Changes to support downstream clone #2197

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 10 commits into from
Apr 16, 2025
Merged

Conversation

liamappelbe
Copy link
Contributor

An internal downstream clone needs to change the behavior of a few functions. This PR moves them all to a single overrideable_utils.dart file so that the clone can just swap out that file, rather than modifying the functions in place.

Copy link

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@coveralls
Copy link

coveralls commented Apr 14, 2025

Coverage Status

coverage: 85.395% (-0.001%) from 85.396%
when pulling b8cda69 on ffigen_upstream
into 0d57838 on main.

@liamappelbe liamappelbe marked this pull request as draft April 14, 2025 05:06

import 'package:path/path.dart' as p;

// Replaces the path separators according to current platform.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about interface class PathHandler and class DefaultPathHandler in the main function? That way it's a single constructor call in the main function. And we we use a different main function downstream there is no need for a git patch at all. (This is how the patching for native assets in flutter tools in g3 works.)

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'm pretty sure ffigen uses the same main function in g3 as it does externally, so I'd have to write a patch file either way

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't we importing overrideable_utils.dart in multiple places leading to multiple patches? Ah, you work around that by using an export of that file and never importing it anywhere else.

Other contributors to this project might easily autocomplete an import statement to overrideable_utils.dart which would then require another patch. So, if you want to go that route maybe add a @Deprecated() library; at the top to prevent contributors from ever importing this file anywhere.

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

@liamappelbe liamappelbe marked this pull request as ready for review April 16, 2025 01:50
@liamappelbe liamappelbe requested a review from dcharkes April 16, 2025 01:50
@liamappelbe liamappelbe merged commit a796a45 into main Apr 16, 2025
23 checks passed
@liamappelbe liamappelbe deleted the ffigen_upstream branch April 16, 2025 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants