馃 chore: sync skills directory from dart-lang/skills - #241
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Dart and Flutter plugin versions to 1.0.6 and enhances the Dart path manipulation and pattern matching skill guides to recommend Dart 3 list pattern matching (e.g., if-case with rest elements) for path segment inspection instead of manual indexing. Feedback on these changes suggests addressing edge cases where absolute paths might fail prefix pattern matching, correcting a checklist item that conflates prefix matching with containment checks, and using robust path normalization or joining methods instead of rest.join('/') in the pattern matching example.
| * **Prefer**: `if (p.split(path) case ['foo', ...])` (or `case ['foo', ...final rest]` when extracting tail segments), or `p.isWithin('foo', path)` | ||
| * **Avoid**: `path.startsWith('foo/')` or `p.split(path).first == 'foo'` | ||
| * **Why**: String prefix matching fails on Windows separators (`foo\bar`). Calling | ||
| `p.split(path).first` throws a `StateError` on empty lists and requires | ||
| separate `.skip(1)` slicing, whereas list patterns safely check non-emptiness, | ||
| match multi-segment prefixes, and optionally bind `...final rest` in a single | ||
| step. When unnormalized relative prefixes like `./foo/bar` may appear, use | ||
| `p.isWithin('foo', path)` (or `p.split(p.normalize(path))`). |
There was a problem hiding this comment.
When dealing with absolute paths, p.split(path) will include the root directory (such as '/' on POSIX or 'C:\\' on Windows) as the first element in the returned list. Consequently, a pattern match like case ['foo', ...] will fail to match absolute paths even if they start with 'foo' relative to the root. To handle absolute paths safely, consider converting them to relative paths using p.relative(path, from: root) before performing list pattern matching, or use p.isWithin.
| ### Path Refactoring Checklist | ||
| - [ ] Replace string interpolation (`'$dir/$file'`) with `p.join(dir, file)`. | ||
| - [ ] Replace `.contains('dir/')` and `.startsWith('dir/')` with `p.split(path)` segment checks or `p.isWithin(parent, child)`. | ||
| - [ ] Replace `.contains('dir/')` and `.startsWith('dir/')` with `p.split(path)` list pattern checks (`case ['dir', ...final rest]`) or `p.isWithin(parent, child)`. |
There was a problem hiding this comment.
The checklist item suggests replacing both .contains('dir/') and .startsWith('dir/') with case ['dir', ...final rest]. However, case ['dir', ...final rest] only matches when 'dir' is the first segment (equivalent to startsWith). To match .contains('dir/'), a pattern like case [..., 'dir', ...] or p.split(path).contains('dir') should be used instead.
| String? resolveAllowedAssetSubpath(List<String> segments) { | ||
| if (segments case ['assets', ...final rest] | ||
| when rest.isNotEmpty && !rest.contains('..')) { | ||
| return rest.join('/'); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
While this is an illustrative example of list pattern matching, using rest.join('/') on unnormalized segments can preserve empty segments (e.g., ['', 'foo'] joining to '/foo') or current directory segments ('.'), which might bypass intended path restrictions or produce malformed paths. For robust path resolution, consider normalizing the path using p.normalize or joining via p.posix.joinAll(rest).
|
FYI @kevmoo there's some more feedback on dart-lang/skills#49 from Gemini for your consideration. |
2 participants
Automated changes by create-pull-request GitHub action