-
Notifications
You must be signed in to change notification settings - Fork 68
[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
Conversation
PR HealthBreaking changes ✔️
Changelog Entry ✔️
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.
License Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
|
|
||
import 'package:path/path.dart' as p; | ||
|
||
// Replaces the path separators according to current platform. |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.