-
Notifications
You must be signed in to change notification settings - Fork 76
Prepare code_builder 4.11.1 for fixnum 1.2.0.
#2281
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
base: main
Are you sure you want to change the base?
Conversation
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
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.
Code Review
This pull request introduces a targeted fix to handle upcoming changes in fixnum v1.2.0 by rewriting imports from package:fixnum/src/* to package:fixnum/fixnum.dart. The changes are implemented in the Allocator by introducing a _fixUrl helper function, and the package version is updated accordingly. The changes are well-tested and documented in the changelog. My feedback includes a suggestion to improve maintainability by extracting magic strings into constants, following Effective Dart guidelines.
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with 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.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
|
Happy to rubberstamp, but why would this fix be needed? Imports of |
Do we plan to back out the fix in the future? If it's temporary I think I'd prefer landing it in google3 and noted as a local modification, then landing the long term fix here. My initial reaction is that we should push the mapping further towards whatever codegen is choosing that URI, but I think I need to read through the internal bug thread to figure out why we aren't doing that. |
|
|
||
| /// Applies hardcoded fixes to [url]. | ||
| String _fixUrl(String url) { | ||
| if (url.startsWith('package:fixnum/src/')) { |
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.
Can we add a link to the issue here? This feels like the kind of crazy that we forget why we did in 3 years.
|
After reading the thread I'm not convinced that a hardcoded string based fix in |
|
Thanks everyone! This is a fun one. Let's discuss on the internal issue, I'll add everyone. |
| ## 4.11.1-wip | ||
| ## 4.11.1 | ||
|
|
||
| * Convert imports of implementation libraries under `package:fixnum/src/*` into |
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.
Can we also mention that this is expected to be removed in some future release?
This is currently blocking fixnum rolls to google3; having discussed various fixes/workarounds, a highly targeted hard coded fix seems appealing :) ... what do you think, please?