[XL] Change in dart/sdk[main]: [dwds][dwds_test_common] Migrate package to the SDK repository
Jessy Yameogo (Gerrit)
Jessy Yameogo added 1 comment![]( "Open in Gerrit")
Jessy YameogoAre we expecting any tests to run on the trybots showing green on this CL? I spot checked a few because I was curious what is running and didn't see any new test results.
Jessy YameogoCurrently, none of the new tests are running because this package depends on DWDS, which hasn't been migrated yet.
The strategy is to keep the migration manageable by splitting it into separate, smaller CLs rather than one massive change. This specific CL won't be submitted as-is; instead, I’m creating child CLs for the other components so they can be reviewed independently. Once the DWDS migration is ready in a follow-up CL, we’ll be able to see the test results greening up on the trybots. I plan to land all related parts simultaneously once the full migration is complete.
Done
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jessy Yameogo (Gerrit)
Jessy Yameogo abandoned this change.
Jessy Yameogo abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ivan Inozemtsev (Gerrit)
Ivan Inozemtsev voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Ben Konyi
- Jessy Yameogo
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 4 comments![]( "Open in Gerrit")
Should this file live under `dwds/test` since it's only meant to be used in tests?
Future<String> absolutePath(String pathFromDwds) async {`absolutePath` sounds like it's just turning the path into an absolute one. Maybe call this `dwdsPath`?
sse: ^4.2.0-wipShould we really be updating this to a `wip` version?
final resolvedExecutable = executable.contains(p.separator)Can we just always do `File(executable).absolute.path`? I'm unclear on what checking for the separator is doing.
Related details
- Ben Konyi
- Jessy Yameogo
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 4 comments![]( "Open in Gerrit")
exclude:In general I'm opposed to skipping analysis for files that are in the dart sdk repo. It is very common for language features to roll out in the SDK first, and the language team does that by finding analysis errors or new lint violations and sending fixes for them.
I expect we will hear from a confused language team member at some point when they find out they broke our code on a post submit test but there were no analysis errors.
- "lib/data/*"These are written by hand now right? We shouldn't exclude them from analysis.
- "lib/src/handlers/injected_client_js.dart"I know there isn't much dart code here to anlayze but it is still checked in. Do we need to skip analysis on it?
- "test/integration/fixtures/*"Do all the fixtures rely on package:build_daemon?
Would it be possible to reduce the scope of the skips to only the necessary libraries?
Nicholas Shahan (Gerrit)
Nicholas Shahan added 1 comment![]( "Open in Gerrit")
Just leaving a reminder here to rebase this on top of the parent change one last time when you are ready to submit.
Then you should have the option on this CL to submit the entire stack in one operation. After submission they will still appear in the commit history as multiple changes.
Ben Konyi (Gerrit)
Ben Konyi added 5 comments![]( "Open in Gerrit")
exclude:In general I'm opposed to skipping analysis for files that are in the dart sdk repo. It is very common for language features to roll out in the SDK first, and the language team does that by finding analysis errors or new lint violations and sending fixes for them.
I expect we will hear from a confused language team member at some point when they find out they broke our code on a post submit test but there were no analysis errors.
+1, let's skip analysis on as little as possible.
Should this file live under `dwds/test` since it's only meant to be used in tests?
+1
// @skip_package_deps_validationWhat does this do?
Copyright 2019, the Dart project authors.Just checking, is this the right copyright year?
dwds/test/integration/*: SkipByDesignLet's leave some comments here to explain why we're skipping these.
Related details
- Jessy Yameogo
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |