Add StorageFile extensions #481

ghost · 2016-10-16T13:44:19Z

Would be nice if someone could check the comments, as there might be minor language issues.

TODO:

Documentation
Unit tests

dnfclas · 2016-10-16T13:44:23Z

Hi @jlnostr, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

ghost · 2016-10-16T13:59:07Z

I'm thinking about only making the FileExistsAsync task with the ìsRecursive option available to the user, the other ones should be internal. This would result in just one extension instead of three. What do you think about this change?

ScottIsAFool · 2016-10-16T14:36:45Z

Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs

+        internal static async Task<bool> FileExistsInSubtreeAsync(StorageFolder rootFolder, string fileName)
+        {
+            if (fileName.IndexOf('"') >= 0)
+            {


Use nameof()

ScottIsAFool · 2016-10-16T14:37:12Z

Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs

+        /// </param>
+        /// <returns>
+        /// Returns true, if the file exists.
+        /// </returns>


Not the exception that is being thrown

ScottIsAFool · 2016-10-16T14:37:28Z

Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs

+        /// <returns>
+        /// Returns true, if the file exists.
+        /// </returns>
+        /// <exception cref="ArgumentNullException">


Nor the reason

ScottIsAFool · 2016-10-16T14:39:27Z

Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs

+        /// </param>
+        /// <returns>
+        /// Returns true, if the file exists.
+        /// </returns>


Remove the async keyword here, then remove the awaits below it.

ghost · 2016-10-16T17:55:20Z

Applied the requested changes, thanks for the review @ScottIsAFool.

deltakosh · 2016-10-17T18:28:36Z

for the sake of coherence, I'm wondering if we should provide shortcut methods like:

FileExistsInLocalFolderAsync
FileExistsInLocalCacheAsync
FileExistsInKnownFolderAsync

ghost · 2016-10-18T08:03:40Z

Sounds like a good idea to me 👍

ScottIsAFool · 2016-10-18T09:22:15Z

@deltakosh those wouldn't be extension methods though, right? Or would they be extension methods of ApplicationData? If so, I'm not sure I see the benefit.

andrewleader · 2016-10-18T16:25:37Z

Agreed with Scott, I'm not seeing a clear benefit.

ApplicationData.Current.LocalFolder.FileExistsAsync("tacos.txt")

and

StorageFileHelpers.FileExistsInLocalFolderAsync("tacos.txt")

are quite equivalent when it comes to the developer API level. I don't see any clear benefit of the second option, especially when it means we have to implement 5 different methods (Local, LocalCache, Roaming, Temp, Known)

deltakosh · 2016-10-18T16:30:13Z

I see a benefit because you have everything under the same helper. It is just nice to have as it does not require advance coding.
perhaps it is my "I need a 100% completion" problem :)

deltakosh · 2016-10-18T16:31:00Z

docs/helpers/StorageFiles.md

 	loadedText = await StorageFileHelper.ReadTextFromLocalFileAsync("appFilename.txt");

+	// Check if a file exists in a specific folder
+	bool exists = await StorageFileHelper.FileExistsAsync(localFolder, "appFilename.txt");


Should we demonstrate this version or the extension? (I'm more for the extension)

Yeah, might be the better solution. Will modify it :)

ScottIsAFool · 2016-10-18T16:39:46Z

@deltakosh I'm not sure you're going to convince me of this :)

deltakosh · 2016-10-18T16:42:23Z

I'm not trying to hard to be honest. I'm fine with just the extension!

deltakosh · 2016-10-18T16:42:59Z

I only have one comment in the code and then I'm fine to merge

ghost · 2016-10-18T17:45:03Z

and then I'm fine to merge

So, no unit tests then?

deltakosh · 2016-10-18T17:47:27Z

I'm fine to merge (but only when unit tests and doc will be provided)
:D

ghost · 2016-10-18T17:57:20Z

While writing the unit tests I realized that there's a IsFileExistsAsync extension in StreamHelpers.cs already (see here). What to do with it? Keeping it as a duplicate doesn't make sense to me. Honestly, I don't even understand what it has to do in StreamHelpers.

deltakosh · 2016-10-18T18:01:02Z

OMG, I completely forgot about this.
We have two options now: Keep them here but it does not make a lot of sense but it saves the back compat OR deprecate them and point them to the StorageFileHelper version (my favorite)

ghost · 2016-10-18T18:04:45Z

Because of compatibility, making the old helpers obsolete is the better solution IMO.

ghost · 2016-10-18T18:05:28Z

Should I delete the related unit tests as well?

deltakosh · 2016-10-18T18:05:34Z

Done deal(and you can then remove them from the unit tests, mark them as deprecate and just use the StorageFileHelper internally

ghost · 2016-10-18T18:07:28Z

Will do!

deltakosh · 2016-10-20T17:43:47Z

UnitTests/Helpers/Test_StreamHelper.cs

        {
-            Assert.IsTrue(await StreamHelper.IsPackagedFileExistsAsync(PackagedFilePath));
+            var packageFolder = Package.Current.InstalledLocation;
+            Assert.IsTrue(await StorageFileHelper.FileExistsAsync(packageFolder, PackagedFilePath));


Do you mind using extensions instead? The unit tests are also used to demonstrate the feature

Yeah, no problem. But every other test is it using the above way and I want to keep a bit of consistency.

Actually I mentioned this for all tests:)

deltakosh · 2016-10-20T17:44:53Z

Microsoft.Toolkit.Uwp/Helpers/StreamHelper.cs

+        [Obsolete("Use StorageFileHelper.FileExistsAsync instead.")]
        public static async Task<bool> IsFileExistsAsync(
            this StorageFolder workingFolder,
            string fileName)


We should rely on the StorageFileHelper instead of duplicating the code?

It's very confusing due to the similar names. I marked it as Obsolete so devs have time to adapt the changes. With the 1.3 release we could probably remove the IsFileExistsAsync method from the StreamHelper.

Not sure to understand why it is confusing. We will for sure remove it in next version

A developer might ask: Should I use IsFileExistsAsync or FileExistsAsync? What's the difference? Why is the syntax different? Is one better than another?

As IsFileExistsAsync is marked obsolete I think this is already a good reason no to use it

hermitdave

Looks good otherwise...

hermitdave · 2016-10-24T22:07:09Z

UnitTests/Helpers/Test_StreamHelper.cs

-            Assert.IsTrue(await StreamHelper.IsKnownFolderFileExistsAsync(folder, Filename));
+            Assert.IsTrue(await knownFolder.FileExistsAsync(Filename));

            using (var stream = await StreamHelper.GetKnowFoldersFileStreamAsync(folder, Filename))


If StreamHelper methods are now obsolete, should we move the tests elsewhere ?

If the methods are obsolete, shouldn't we delete the tests?

ScottIsAFool · 2016-10-26T22:48:42Z

UnitTests/Helpers/Test_StreamHelper.cs

-            Assert.IsTrue(await StreamHelper.IsKnownFolderFileExistsAsync(folder, Filename));
+            Assert.IsTrue(await knownFolder.FileExistsAsync(Filename));

            using (var stream = await StreamHelper.GetKnowFoldersFileStreamAsync(folder, Filename))


If the methods are obsolete, shouldn't we delete the tests?

deltakosh · 2016-10-26T22:53:29Z

agree with Scott

hermitdave · 2016-10-27T09:00:32Z

@jlnostr can we delete the old tests for methods marked obsolete. Please add a few that directly use the new methods and name those accordingly ?

deltakosh · 2016-10-28T22:57:07Z

Ping? I guess we are almost done to merge this one

dotMorten · 2016-10-28T23:14:29Z

Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs

+                FolderDepth = FolderDepth.Deep,
+                UserSearchFilter = $"filename:=\"{fileName}\"" // “:=” is the exact-match operator
+            };
+


No reason for thread switching. Should have .ConfigureAwait(false) at the end.

dotMorten · 2016-10-28T23:14:39Z

Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs

+        /// Returns true, if the file exists.
+        /// </returns>
+        internal static async Task<bool> FileExistsInFolderAsync(StorageFolder folder, string fileName)
+        {


No reason for thread switching. Should have .ConfigureAwait(false) at the end

deltakosh · 2016-10-31T20:39:11Z

Up? I would like to merge this one for 1.2

ScottIsAFool · 2016-11-03T14:42:43Z

@jlnostr I have made the requested changes and merged your PR in.

Julian Oster added 2 commits October 16, 2016 15:40

Add StorageFile extensions.

20d8c57

See #470.

Update naming to follow the rest of the class.

390ac7e

dnfclas added the cla-already-signed label Oct 16, 2016

ghost changed the title ~~Add StorageFile extensions~~ [WIP] Add StorageFile extensions Oct 16, 2016

Julian Oster added 2 commits October 16, 2016 15:55

Slightly improve the summary.

e8d00d8

Add documentation.

84002db

Julian Oster added 2 commits October 16, 2016 16:02

Simplify the FileExists extensions by only making one public.

b798939

Move the public extension above the internal ones.

760517b

ScottIsAFool reviewed Oct 16, 2016

View reviewed changes

Add requested changes.

b07b689

deltakosh reviewed Oct 18, 2016

View reviewed changes

Make use of FileExists as extension.

a569326

Add unit tests.

0f5eaa8

Julian Oster added 3 commits October 18, 2016 20:20

Make FileExists extensions from StreamHelper obsolete.

b70fc7a

Update unit tests for StreamHelper.

37c5185

Use StorageFileHelper in BaseObjectStorageHelper.

ca5ae9f

ghost changed the title ~~[WIP] Add StorageFile extensions~~ Add StorageFile extensions Oct 18, 2016

Resolve conflicts.

a43c7e2

deltakosh added need documentation 📃 reviewers wanted and removed need documentation 📃 labels Oct 18, 2016

deltakosh approved these changes Oct 18, 2016

View reviewed changes

deltakosh reviewed Oct 20, 2016

View reviewed changes

Rewrite unit tests and use extensions wherever possible.

ebef9a1

hermitdave reviewed Oct 24, 2016

View reviewed changes

ScottIsAFool approved these changes Oct 26, 2016

View reviewed changes

dotMorten reviewed Oct 28, 2016

View reviewed changes

ScottIsAFool merged commit ebef9a1 into CommunityToolkit:dev Nov 3, 2016

Add StorageFile extensions #481

Add StorageFile extensions #481

Uh oh!

Conversation

ghost commented Oct 16, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Oct 16, 2016

Uh oh!

ghost commented Oct 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 16, 2016

Uh oh!

deltakosh commented Oct 17, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

ScottIsAFool commented Oct 18, 2016

Uh oh!

andrewleader commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deltakosh commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScottIsAFool commented Oct 18, 2016

Uh oh!

deltakosh commented Oct 18, 2016

Uh oh!

deltakosh commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

deltakosh commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

deltakosh commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

deltakosh commented Oct 18, 2016

Uh oh!

ghost commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermitdave left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ghost commented Oct 16, 2016 •

edited by ghost

Loading

andrewleader commented Oct 18, 2016 •

edited

Loading