GH1564: Allow deletion of read-only files #1574

DixonDs · 2017-04-18T11:37:13Z

dnfclas · 2017-04-18T11:37:17Z

@DixonDs,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

bjorkstromm · 2017-04-18T18:36:20Z

@DixonDs
Thanks for your PR.

We have been discussing this internally and would rather see this behavior as opt-in. What do you think of creating overloads for all DeleteDirectory aliases?

Preferred solution could be:

DeleteDirectories(ICakeContext, IEnumerable<DirectoryPath>, DeleteDirectorySettings)
DeleteDirectories(ICakeContext, IEnumerable<string>, DeleteDirectorySettings)
DeleteDirectory(ICakeContext, DirectoryPath, DeleteDirectorySettings)

Where DeleteDirectorySettings could look like this:

public class DeleteDirectorySettings
{
   public bool Recursive { get; set; }
   public bool Force { get; set; }
}

toddstewart · 2017-04-18T18:44:57Z

I like the idea of the Force property but you have to take into mind files that are in use. I have run into where a file in the packages directory is in use and cannot be deleted.

bjorkstromm

This behavior should be opt-in. Please create overloaded aliases where it's possible to remove read-only files. See Comment above

toddstewart · 2017-04-18T18:56:14Z

Yes I agree with opt-in but if the property is called Force it does not indicate it is only to delete read only files. Just wanted to point that out.

bjorkstromm · 2017-04-18T19:00:53Z

@toddstewart sorry, didn't see your comment. Just submitted a review comment 😄

Is there any better name to use than force? Force is quite commonly used in same situtations.. Even though it's called force, it might fail miserably if a) user doesn't have rights to remove files/directories, b) files/directories are in use c) anything else..

devlead · 2017-04-18T19:37:00Z

@mholo65 @toddstewart I think Force is an excellent name.

DixonDs · 2017-04-19T07:01:52Z

@mholo65 I've added overloads with DeleteDirectorySettings, but removed the ones with bool recursive which is a breaking change. Let me know if I need to put back those overloads with the boolean recursive flag.

bjorkstromm

@DixonDs yes, don't delete the aliases with bool recursive, becouse as you said, this will be a breaking change.

Just add new aliases with DeleteDirectorySettings settings and let the old aliases (with bool recursive) call the new alias with correct settings.

bjorkstromm · 2017-04-19T07:52:40Z

src/Cake.Core/IO/IDirectory.cs

@@ -32,7 +32,8 @@ public interface IDirectory : IFileSystemInfo
        /// Deletes the directory.
        /// </summary>
        /// <param name="recursive">Will perform a recursive delete if set to <c>true</c>.</param>
-        void Delete(bool recursive);
+        /// <param name="force">Will delete read-only files if set to <c>true</c></param>
+        void Delete(bool recursive, bool force);


This is also a breaking change.

@mholo65 I am not sure if there is anything that can be done here to avoid a breaking change. Even I added a separate Delete overload, that would be a breaking change since all implementations would have to implement that new method from the interface

Indeed all interface changes are breaking changes, this would break a few addins an modules. When possible use extension methods or alternatively we need to set a minimum supported cake version going forward.

@DixonDs yes, exactly.
Just wondering if the attribute modification should be done elsewhere? E.g. introduce new interface for that.

Thoughts on this @patriksvensson @devlead

@mholo65 don't want to litter code base with allot of interfaces either, so one option could be to get this in a milestone/release whilst we have other breaking changes that need a minimum cake version set, i.e. the bigger roslyn refactoring 0.21:isch time frame.

If doing so that would involve IFile::Delete() too.

@devlead @mholo65 It could be done in internal DirectoryDeleter if IFile provided access to file attributes but it doesn't. But if exposing getting/setting attributes in IFile deserves a breaking change on its own, that could be an alternative way to go.

Please, advise

@devlead Sounds good to me (i.e. having merged in 0.21) when we have other breaking changes.

@DixonDs IFileprovides the Path so you can create a System.IO.FileInfo from that.

@mholo65 Doesn't that defeat the purpose of abstraction? WillDirectoryAliasesTests work where the fake file system is used?

devlead

There's a few code style issues too

.\src\Cake.Common\IO\DeleteDirectorySettings.cs(1,1): error SA1633: The file header is missing or not located at the top of the file.
.\src\Cake.Common\IO\DeleteDirectorySettings.cs(11,21): error SA1623: The property's documentation summary text must begin with: 'Gets or sets a value indicating whether'
.\src\Cake.Common\IO\DeleteDirectorySettings.cs(16,21): error SA1623: The property's documentation summary text must begin with: 'Gets or sets a value indicating whether'

Running release build should surface these locally.

DixonDs · 2017-04-30T13:15:35Z

@mholo65 @devlead Could you please review the latest changes?

bjorkstromm

Some minor cosmetic changes.

Could you also add tests which proves that attributes are modified when using Force option.

bjorkstromm · 2017-05-01T19:12:35Z

src/Cake.Common/IO/DirectoryAliases.cs

@@ -67,7 +67,39 @@ public static ConvertableDirectoryPath Directory(this ICakeContext context, stri
        /// <param name="recursive">Will perform a recursive delete if set to <c>true</c>.</param>
        [CakeMethodAlias]
        [CakeAliasCategory("Delete")]
-        public static void DeleteDirectories(this ICakeContext context, IEnumerable<DirectoryPath> directories, bool recursive = false)
+        public static void DeleteDirectories(this ICakeContext context, IEnumerable<DirectoryPath> directories,
+            bool recursive = false)


Could you move this back up to the previous line.

bjorkstromm · 2017-05-01T19:14:12Z

src/Cake.Common/IO/DirectoryAliases.cs

+        public static void DeleteDirectories(this ICakeContext context, IEnumerable<DirectoryPath> directories,
+            bool recursive = false)
+        {
+            if (directories == null)


Remove redundant null check. This is also checked in overload.

bjorkstromm · 2017-05-01T19:15:18Z

src/Cake.Common/IO/DirectoryAliases.cs

@@ -97,7 +129,39 @@ public static void DeleteDirectories(this ICakeContext context, IEnumerable<Dire
        /// <param name="recursive">Will perform a recursive delete if set to <c>true</c>.</param>
        [CakeMethodAlias]
        [CakeAliasCategory("Delete")]
-        public static void DeleteDirectories(this ICakeContext context, IEnumerable<string> directories, bool recursive = false)
+        public static void DeleteDirectories(this ICakeContext context, IEnumerable<string> directories,
+            bool recursive = false)


Could you move this back up to the previous line.

bjorkstromm · 2017-05-01T19:16:02Z

src/Cake.Common/IO/DirectoryAliases.cs

+        public static void DeleteDirectories(this ICakeContext context, IEnumerable<string> directories,
+            bool recursive = false)
+        {
+            if (directories == null)


Remove redundant null check. This is also checked in overload.

bjorkstromm · 2017-05-01T19:24:36Z

src/Cake.Testing/FakeFile.cs

@@ -158,9 +164,11 @@ private long GetPosition(FileMode fileMode, out bool fileWasCreated)
                    case FileMode.Truncate:
                        Length = 0;
                        return 0;
+


Remove extra line

bjorkstromm · 2017-05-01T19:24:48Z

src/Cake.Testing/FakeFile.cs

                    case FileMode.Open:
                    case FileMode.OpenOrCreate:
                        return 0;
+


Remove extra line

bjorkstromm · 2017-05-01T19:24:59Z

src/Cake.Testing/FakeFile.cs

@@ -176,6 +184,7 @@ private long GetPosition(FileMode fileMode, out bool fileWasCreated)
                        fileWasCreated = true;
                        Exists = true;
                        return Length;
+


Remove extra line

bjorkstromm · 2017-05-01T19:25:55Z

src/Cake.Common/IO/DirectoryAliases.cs

+        [CakeMethodAlias]
+        [CakeAliasCategory("Delete")]
+        public static void DeleteDirectory(this ICakeContext context, DirectoryPath path,
+            DeleteDirectorySettings settings)


Move this up one line

bjorkstromm · 2017-05-01T19:26:07Z

src/Cake.Common/IO/DirectoryAliases.cs

+        [CakeMethodAlias]
+        [CakeAliasCategory("Delete")]
+        public static void DeleteDirectories(this ICakeContext context, IEnumerable<string> directories,
+            DeleteDirectorySettings settings)


Move this up one line

bjorkstromm · 2017-05-01T19:26:22Z

src/Cake.Common/IO/DirectoryAliases.cs

+        [CakeMethodAlias]
+        [CakeAliasCategory("Delete")]
+        public static void DeleteDirectories(this ICakeContext context, IEnumerable<DirectoryPath> directories,
+            DeleteDirectorySettings settings)


Move this up one line

DixonDs · 2017-05-16T18:10:04Z

@mholo65 @devlead Could you please review again?

bjorkstromm · 2017-05-16T18:21:08Z

@DixonDs

Could you please review again?

Looks good to me. I was thinking if we should obsolote the old aliases at the same time. Thoughts @cake-build/cake-team?

If @devlead or any other in team doesn't have any opinions on this, would you @DixonDs mind squashing commits before we do the final review. Thanks!

DixonDs · 2017-05-16T19:05:25Z

@mholo65

would you @DixonDs mind squashing commits before we do the final review

I will do later, but I am just wondering if it makes much sense if you can just complete PR with merge-squash.

gep13 · 2017-05-16T19:07:12Z

@DixonDs no, that is not the process that we follow on this repository, as the git timeline isn't as clear.

DixonDs · 2017-05-25T11:00:14Z

@mholo65 Could you please do the final review?

bjorkstromm

@DixonDs Looks good to me. Thanks for your contribution.

As this PR introduces a breaking change in Cake.Core, which will most likely break some addins and modules, we won't merge it for the upcoming release. Instead I'll put this on hold and get back to this for the 0.21.0 release.

bjorkstromm

LGTM!

patriksvensson · 2017-07-29T20:12:50Z

src/Cake.Common/IO/DeleteDirectorySettings.cs

+        /// <summary>
+        /// Gets or sets a value indicating whether to delete read-only files if set to <c>true</c>.
+        /// </summary>
+        public bool Force { get; set; }


Perhaps we should add <remarks> here that this is false by default.

bjorkstromm · 2017-07-29T20:15:27Z

src/Cake.Common/IO/DirectoryAliases.cs

@@ -99,6 +129,31 @@ public static void DeleteDirectories(this ICakeContext context, IEnumerable<Dire
        [CakeAliasCategory("Delete")]
        public static void DeleteDirectories(this ICakeContext context, IEnumerable<string> directories, bool recursive = false)


@Cake-build/team should we obsolete these?

Ping @cake-build/cake-team

@mholo65 Yes, I think we should. Obsolete with a warning message for now.

@mholo65 I am on the fence on this one. They are still valid, as they can be easily mapped into the new way of doing things, so part of me says to leave them. Having said that, would also be happy to see them removed.

@gep13 yes, leave them for a release or two. But obsolete with warning 😉

bjorkstromm · 2017-07-30T22:20:59Z

@DixonDs are you in position to rebase and fix changes requested? Thanks!

DixonDs · 2017-07-31T07:42:13Z

@mholo65 done

devlead · 2017-07-31T14:57:25Z

@DixonDs CS0618 needs to be disabled case by case in the test project

i.e.

#pragma warning disable CS0618 // Type or member is obsolete
                var result = Record.Exception(() => DirectoryAliases.DeleteDirectory(null, "/Temp/DoNotExist"));
#pragma warning restore CS0618 // Type or member is obsolete

devlead

LGTM I'll merge once CI finishes.

devlead · 2017-08-01T13:02:37Z

@DixonDs your changes have been merged, thanks for your contribution 👍

dnfclas added the cla-already-signed label Apr 18, 2017

devlead requested a review from bjorkstromm April 18, 2017 18:48

bjorkstromm requested changes Apr 18, 2017

View reviewed changes

devlead changed the title ~~Allow deletion of read-only files~~ GH1564: Allow deletion of read-only files Apr 18, 2017

bjorkstromm requested changes Apr 19, 2017

View reviewed changes

bjorkstromm reviewed Apr 19, 2017

View reviewed changes

devlead requested changes Apr 19, 2017

View reviewed changes

bjorkstromm requested changes May 1, 2017

View reviewed changes

DixonDs force-pushed the feature-delete-dir branch from fa56863 to 1d169f5 Compare May 16, 2017 19:46

bjorkstromm self-assigned this May 26, 2017

bjorkstromm reviewed May 26, 2017

View reviewed changes

bjorkstromm changed the title ~~GH1564: Allow deletion of read-only files~~ [HOLD] GH1564: Allow deletion of read-only files May 26, 2017

DixonDs force-pushed the feature-delete-dir branch from 1d169f5 to 7cf28af Compare June 29, 2017 11:44

bjorkstromm changed the title ~~[HOLD] GH1564: Allow deletion of read-only files~~ GH1564: Allow deletion of read-only files Jul 29, 2017

bjorkstromm approved these changes Jul 29, 2017

View reviewed changes

patriksvensson reviewed Jul 29, 2017

View reviewed changes

bjorkstromm reviewed Jul 29, 2017

View reviewed changes

DixonDs force-pushed the feature-delete-dir branch from 7cf28af to 48dafcf Compare July 31, 2017 07:41

DixonDs force-pushed the feature-delete-dir branch from 48dafcf to 8c8cf3f Compare August 1, 2017 06:52

devlead approved these changes Aug 1, 2017

View reviewed changes

devlead force-pushed the feature-delete-dir branch from 8c8cf3f to 2a2c417 Compare August 1, 2017 11:21

devlead approved these changes Aug 1, 2017

View reviewed changes

Allow deletion of read-only files

ba42369

DixonDs force-pushed the feature-delete-dir branch from 2a2c417 to ba42369 Compare August 1, 2017 11:42

devlead merged commit ba42369 into cake-build:develop Aug 1, 2017

DixonDs deleted the feature-delete-dir branch August 1, 2017 13:02

bjorkstromm mentioned this pull request Aug 21, 2017

CleanDirectory does not clean readonly files. #1317

Closed

FrankRay78 mentioned this pull request Nov 22, 2022

Ability to force clean readonly files #4077

Merged

		@@ -99,6 +129,31 @@ public static void DeleteDirectories(this ICakeContext context, IEnumerable<Dire
		[CakeAliasCategory("Delete")]
		public static void DeleteDirectories(this ICakeContext context, IEnumerable<string> directories, bool recursive = false)

GH1564: Allow deletion of read-only files #1574

GH1564: Allow deletion of read-only files #1574

Conversation

DixonDs commented Apr 18, 2017

dnfclas commented Apr 18, 2017

bjorkstromm commented Apr 18, 2017

toddstewart commented Apr 18, 2017

bjorkstromm left a comment • edited Loading

Choose a reason for hiding this comment

toddstewart commented Apr 18, 2017

bjorkstromm commented Apr 18, 2017

devlead commented Apr 18, 2017

DixonDs commented Apr 19, 2017

bjorkstromm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DixonDs Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorkstromm Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devlead left a comment

Choose a reason for hiding this comment

DixonDs commented Apr 30, 2017

bjorkstromm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DixonDs commented May 16, 2017

bjorkstromm commented May 16, 2017

DixonDs commented May 16, 2017

gep13 commented May 16, 2017

DixonDs commented May 25, 2017

bjorkstromm left a comment

Choose a reason for hiding this comment

bjorkstromm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorkstromm commented Jul 30, 2017

DixonDs commented Jul 31, 2017

devlead commented Jul 31, 2017

devlead left a comment

Choose a reason for hiding this comment

devlead commented Aug 1, 2017

bjorkstromm left a comment •

edited

Loading

DixonDs Apr 19, 2017 •

edited

Loading

bjorkstromm Apr 19, 2017 •

edited

Loading