Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FileSaverResult and FolderPickerResult, Fix Initial folder on Android #1009

Merged
merged 19 commits into from
Feb 27, 2023

Conversation

VladislavAntonyuk
Copy link
Collaborator

@VladislavAntonyuk VladislavAntonyuk commented Feb 17, 2023

Description of Change

Add new safe methods that return result instead of throwing exception.
A;so fixes initial path on Android

PR Checklist

Additional information

@VladislavAntonyuk VladislavAntonyuk requested a review from a team February 17, 2023 19:51
Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VladislavAntonyuk I've a couple of requests and suggestions (:

Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vlad! Just a thought about adding an .EnsureSuccess() method, similar to HttpResponseMethod.EnsureSuccessStatusCode()

@VladislavAntonyuk VladislavAntonyuk self-assigned this Feb 19, 2023
@brminnick
Copy link
Collaborator

brminnick commented Feb 21, 2023

Hey Vlad! After giving this some thought, let's re-write the existing SaveAsync() methods' behavior instead of creating new SaveSafeAsync() APIs.

I know it's a breaking change, but having one API that returns FIleSaverResult / FolderPickerResult is a better API surface in the long run for our developers to consume, and it's less confusing for developers who would be trying to figure out the difference between SaveAsync() and SaveSafeAsync() and when to choose one vs the other.

Plus, we have to bump our major version for our next release anyways because of a breaking change we've already merged in a different PR.

Here's what I'm thinking:

  • Move logic from SaveSafeAsync() methods to SaveAsync()
  • Remove the SaveSafeAsync() methods

Does that make sense?

For example, FileSaver.SaveAsync() would look like this:

public Task<FileSaverResult> SaveAsync(string fileName, Stream stream, CancellationToken cancellationToken)
{
   try
   {
 	  cancellationToken.ThrowIfCancellationRequested();
 	  // existing SaveAsync logic here
 	  return new FileSaverResult(path, null);
   }
   catch (Exception e)
   {
 	  return new FileSaverResult(null, e);
   }
}

@brminnick
Copy link
Collaborator

Hey Vlad! What do you think about making changing FolderSaverImplementation and FileSaverImplementation from public to internal?

public sealed partial class FolderSaverImplementation -> sealed partial class FolderSaverImplementation
public sealed partial class FileSaverImplementation -> sealed partial class FileSaverImplementation

I think it might be confusing to devs to see both FolderSaver and FolderSaverImplementation in the docs/intellisense. Plus, we can always make them public later without causing a breaking change.

We'd also have to remove the example from the sample app that demonstrates new *Implemetation().

@pictos
Copy link
Member

pictos commented Feb 23, 2023

@brminnick those are publics because they live on Core, we shouldn't have internal there, since it's intended to be used for other libs or future toolkits

@brminnick
Copy link
Collaborator

@pictos Good point 💯

@brminnick brminnick changed the title Essentials Safe methods, Fix Initial folder on Android Add FileSaverResult and FolderPickerResult, Fix Initial folder on Android Feb 23, 2023
@brminnick brminnick dismissed pictos’s stale review February 26, 2023 21:14

Requested changed addressed

Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Vlad!!

@brminnick brminnick added pending documentation This feature requires documentation do not merge Do not merge this PR approved This Proposal has been approved and is ready to be added to the Toolkit labels Feb 26, 2023
@brminnick
Copy link
Collaborator

brminnick commented Feb 26, 2023

@bijington Are you able to help update the docs for this PR? I've added do not merge Do not merge this PR and pending documentation This feature requires documentation for now to ensure we update the docs.

We modified the return Type of IFileSaver.SaveAsync() + IFolderPicker.PickAsync():

Modified APIs

public interface IFileSaver
{
  Task<FileSaverResult> SaveAsync(string initialPath, string fileName, Stream stream, CancellationToken cancellationToken);
  Task<FileSaverResult> SaveAsync(string fileName, Stream stream, CancellationToken cancellationToken);
}

public class FileSaver
{
  Task<FolderPickerResult> PickAsync(string initialPath, CancellationToken cancellationToken);
  Task<FolderPickerResult> PickAsync(CancellationToken cancellationToken);
}

^@jfversluis FYI - these are breaking changes that we'll need to note in our release notes for v5.0.0

@VladislavAntonyuk
Copy link
Collaborator Author

@brminnick brminnick removed pending documentation This feature requires documentation do not merge Do not merge this PR labels Feb 27, 2023
@VladislavAntonyuk VladislavAntonyuk enabled auto-merge (squash) February 27, 2023 17:17
@VladislavAntonyuk VladislavAntonyuk merged commit e96d57f into main Feb 27, 2023
@VladislavAntonyuk VladislavAntonyuk deleted the dialogs-safe-methods branch February 27, 2023 17:55
@github-actions github-actions bot locked and limited conversation to collaborators Nov 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved This Proposal has been approved and is ready to be added to the Toolkit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants