[io] copyPath/copyPathSync: expose parameter `followLinks` #1267

gmpassos · 2024-12-10T21:50:13Z

Directory.listSync only returns Link entries if followLinks: false is passed.

copyPath needs a followLinks parameter to configure the call of Directory.listSync and allow the copy of Link entries, as expected by line else if (file is Link) {

[✅ ] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

gmpassos · 2024-12-17T01:27:02Z

FYI: @natebosch

natebosch · 2025-01-14T00:30:34Z

@jakemac53 I think uses are unlike to break if we change the default behavior. We already have a default behavior for recursive with no override. Do you think it would make sense to assume followLinks: true too?

github-actions · 2025-01-14T00:32:46Z

PR Health

Breaking changes ✔️

Package	Change	Current Version	New Version	Needed Version	Looking good?
io	Non-Breaking	1.0.5	1.1.0-wip	1.1.0	✔️

Changelog Entry ✔️

Package	Changed Files

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

Coverage ✔️

File	Coverage
pkgs/io/lib/src/copy_path.dart	💚 100 % ⬆️ 18 %

This check for test coverage is informational (issues shown here will not fail the PR).

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/bazel_worker/benchmark/benchmark.dart
pkgs/bazel_worker/example/client.dart
pkgs/bazel_worker/example/worker.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

gmpassos · 2025-01-14T04:49:59Z

@jakemac53 I think uses are unlike to break if we change the default behavior. We already have a default behavior for recursive with no override. Do you think it would make sense to assume followLinks: true too?

The current behavior is followLinks: true due to Directory.list default value for followLinks:
https://api.dart.dev/dart-io/Directory/list.html

I vote to keep the current behavior as the default since this is a core function and could be used by multiple third-party projects that might not catch any changes in this behavior in their tests, as it's not a common thing to test.

The main purpose is to add an extra option for those who really want to change the behavior.

jakemac53

This looks fine to me

natebosch · 2025-01-15T17:50:37Z

After a discussion with a few folks I do think it'll be easier to change the default behavior than to add an argument for this. I'm pretty confident it's the behavior we'd want as default, and omitting the argument will save on testing surface area. I'll open up another PR shortly to do that change.

jakemac53 · 2025-01-15T17:59:34Z

After a discussion with a few folks I do think it'll be easier to change the default behavior than to add an argument for this. I'm pretty confident it's the behavior we'd want as default, and omitting the argument will save on testing surface area. I'll open up another PR shortly to do that change.

I am not quite understanding why we want to change behavior here? Why do we want to diverge from what dart:io does by default?

jakemac53 · 2025-01-15T18:03:24Z

I guess the question really here is do you actually copy the (recursive) file contents the links point to or do you copy the links themselves?

Today we copy the file contents because by default links are followed.

Both seem useful depending on the application - I would probably consider the deep copy to be the "safest" option. You are expecting a copy here and not a pointer (link). If you don't do the deep copy, and the thing the link points to changes, your "copy" actually changes too because all you copied was the pointer.

gmpassos · 2025-01-15T19:48:02Z

I'm pretty confident it's the behavior we'd want as default.

IMHO, you shouldn't change this behavior after 8 years.

omitting the argument will save on testing surface area. I'll open up another PR shortly to do that change.

To avoid testing on the Dart SDK, you could potentially break many third-party tests. Personally, I can't agree with that.

Without the followLinks parameter, depending on the default behavior leads to the following limitations:

Solutions that aim to copy a tree of files without copying links will be unable to use copyPath.
Solutions that require a local copy with links, to avoid file duplication, will be unable to use copyPath.

natebosch · 2025-01-15T23:23:02Z

I guess the question really here is do you actually copy the (recursive) file contents the links point to or do you copy the links themselves?

The intention at the time was that the links would be copied. That's also the behavior that makes the most sense to me, and is what I'd do if I was implementing this today - either being the default if there was a reason to make this optional, or more likely make it the only implementation.

I am not quite understanding why we want to change behavior here?

Primarily:

Reduce the amount of testing surface area. If we were strict about testing, it would cut the surface in half, but in practice it's a smaller savings to avoid the extra boolean argument.
Provide what appears to be the intended behavior to whatever internal use cases need it without them needing to migrate.

natebosch · 2025-01-15T23:41:22Z

See #1972 for the alternative.

natebosch · 2025-01-15T23:46:32Z

what I'd do if I was implementing this today - either being the default if there was a reason to make this optional, or more likely make it the only implementation.

Thinking about it more, if both behaviors were necessary I think I'd still push back against the boolean argument. They feel enough like different operations to me that I'd argue for separating deepCopyLinkedContent which could replace links in place, from copyPath which would always treat links as links. The former could be useful separately.

gmpassos · 2025-01-16T00:07:53Z

deepCopyLinkedContent

I vote for bool deepCopyLinkedContent = true, as it preserves the old behavior while also enabling the new (originally intended) behavior.

natebosch · 2025-01-16T00:29:57Z

I vote for bool deepCopyLinkedContent = true, as it preserves the old behavior while also enabling the new (originally intended) behavior.

I mean that I'd write an entirely separate method deepCopyLinkedContent(String path) {...}. I don't think a boolean argument with any name is the best API design.

gmpassos · 2025-01-16T00:59:13Z

I mean that I'd write an entirely separate method deepCopyLinkedContent(String path) {...}.

Well, it's an alternative, but you are still changing the behavior of copyPath. You will need to check all the internal tools that use it to ensure that nothing breaks.

natebosch · 2025-01-16T01:26:27Z

@jakemac53 I think the followLinks naming is a bit too tied to the implementation. WDYT about the new name and doc? Also, can you take a look at the added tests?

gmpassos · 2025-01-16T02:20:57Z

pkgs/io/test/copy_path_test.dart

+          File(p.join(d.sandbox, _copyDir, linkSource, linkContent));
+      expect(await expectedDir.exists(), isTrue);
+      expect(await expectedFile.exists(), isTrue);
+


Check for the copied content before overwrite it.

gmpassos · 2025-01-16T02:21:23Z

pkgs/io/test/copy_path_test.dart

+          File(p.join(d.sandbox, _copyDir, linkSource, linkContent));
+      expect(await expectedDir.exists(), isTrue);
+      expect(await expectedFile.exists(), isTrue);
+


Same check here, before content overwrite.

gmpassos · 2025-01-16T02:22:21Z

I prefer the parameter name deepCopyLinks, as it is more intuitive. followLinks requires documentation to clarify its actual behavior. followLinks only exists because of the Directory.list parameter, and this shouldn't dictate the copyPath signature.

I have made two comments in the test code.

copyPath/copyPathSync: expose parameter followLinks

2aed0a7

github-actions bot added the package:io label Dec 10, 2024

gmpassos mentioned this pull request Dec 10, 2024

copyPath: add parameter followLinks dart-lang/io#97

Closed

Merge branch 'dart-lang:main' into patch-1

e2de8e3

gmpassos and others added 2 commits December 25, 2024 18:34

Merge branch 'dart-lang:main' into patch-1

56527d9

Add changelog entry

3946b3a

Merge branch 'dart-lang:main' into patch-1

5b4b23d

Add -wip suffix in changelog to match pubspec

305e94d

jakemac53 approved these changes Jan 15, 2025

View reviewed changes

natebosch mentioned this pull request Jan 15, 2025

Don't follow links when copying paths #1972

Closed

natebosch added 3 commits January 16, 2025 01:06

Merge branch 'main' into patch-1

5f6d7cf

Add tests

21a9a74

Update argument name and documentation

a198283

gmpassos commented Jan 16, 2025

View reviewed changes

kevmoo changed the title ~~copyPath/copyPathSync: expose parameter followLinks~~ [io] copyPath/copyPathSync: expose parameter followLinks Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[io] copyPath/copyPathSync: expose parameter `followLinks` #1267

[io] copyPath/copyPathSync: expose parameter `followLinks` #1267

gmpassos commented Dec 10, 2024

gmpassos commented Dec 17, 2024

natebosch commented Jan 14, 2025

github-actions bot commented Jan 14, 2025 •

edited

Loading

gmpassos commented Jan 14, 2025 •

edited

Loading

jakemac53 left a comment

natebosch commented Jan 15, 2025

jakemac53 commented Jan 15, 2025

jakemac53 commented Jan 15, 2025 •

edited

Loading

gmpassos commented Jan 15, 2025 •

edited

Loading

natebosch commented Jan 15, 2025

natebosch commented Jan 15, 2025

natebosch commented Jan 15, 2025 •

edited

Loading

gmpassos commented Jan 16, 2025

natebosch commented Jan 16, 2025

gmpassos commented Jan 16, 2025

natebosch commented Jan 16, 2025

gmpassos Jan 16, 2025

gmpassos Jan 16, 2025

gmpassos commented Jan 16, 2025

[io] copyPath/copyPathSync: expose parameter followLinks #1267

Are you sure you want to change the base?

[io] copyPath/copyPathSync: expose parameter followLinks #1267

Conversation

gmpassos commented Dec 10, 2024

gmpassos commented Dec 17, 2024

natebosch commented Jan 14, 2025

github-actions bot commented Jan 14, 2025 • edited Loading

PR Health

gmpassos commented Jan 14, 2025 • edited Loading

jakemac53 left a comment

Choose a reason for hiding this comment

natebosch commented Jan 15, 2025

jakemac53 commented Jan 15, 2025

jakemac53 commented Jan 15, 2025 • edited Loading

gmpassos commented Jan 15, 2025 • edited Loading

natebosch commented Jan 15, 2025

natebosch commented Jan 15, 2025

natebosch commented Jan 15, 2025 • edited Loading

gmpassos commented Jan 16, 2025

natebosch commented Jan 16, 2025

gmpassos commented Jan 16, 2025

natebosch commented Jan 16, 2025

gmpassos Jan 16, 2025

Choose a reason for hiding this comment

gmpassos Jan 16, 2025

Choose a reason for hiding this comment

gmpassos commented Jan 16, 2025

[io] copyPath/copyPathSync: expose parameter `followLinks` #1267

[io] copyPath/copyPathSync: expose parameter `followLinks` #1267

github-actions bot commented Jan 14, 2025 •

edited

Loading

gmpassos commented Jan 14, 2025 •

edited

Loading

jakemac53 commented Jan 15, 2025 •

edited

Loading

gmpassos commented Jan 15, 2025 •

edited

Loading

natebosch commented Jan 15, 2025 •

edited

Loading