search

SimonSimCity / Xamarin-CrossDownloadManager

A cross platform download manager for Xamarin
MIT License
149 stars 68 forks source link

Change default(string) to string.Empty, updated for Android 9 SDK and enabled support for http #108

Open large opened 5 years ago

large commented 5 years ago

This is my first pull request ever, so be kind if this isn't right 👍

Regarding the string issue discussed her https://github.com/SimonSimCity/Xamarin-CrossDownloadManager/pull/105#issuecomment-472156572

This simple change will ensure that StatusDetails always returns a string. Change eliminate the need for null-checking of strings while using library.

large commented 5 years ago

Have you also had a look at the other platform implementations? And what about the other - not so obvious - situations? E.g. what does a newly created instance of a DownloadFileImplementation have as value? Doesn't it also have null? If you start there, iOS and UWP would also need some work. To give you the next hint: I sometimes forgot resetting the value for StatusDetails when changing the status 😅

I don't have anything else than Android on my dev machine atm. And I no UWP/mac/iphone available for development. Limited resources on my laptop to install it.

On a side note: I personally prefer a distinct separation of concerns in commits. Think of it like this: Using a comma or the word and should get you thinking. Your commit might be solving more than one problem.

I agree! As a newbie in the Github I have much to learn there :)

This last issue will help you a lot when refactoring and you're asked to split a feature apart which you have developed. E.g. now I could've deployed the empty-string story without having to take in the changes related to the Android 9 SDK.

That isn't a problem for me, the changes to Android 9 is what is available at my dev machine atm. But have you ever considered to make a prerelease in nuget? That is a great way for endusers to test the new library and revert if it fails.

SimonSimCity commented 5 years ago

Thanks for getting back to me on this.

Would you be able to at least work down the other use-cases where StatusDetails can contain null? I would then take care of the other platforms once Android is ready.

I have considered publishing pre-releases and I had it working for quite a while, but then the automated CI instance broke and I gave up on it. See https://github.com/SimonSimCity/Xamarin-CrossDownloadManager/commit/564b05ad5c75e02971f6577788468b14d24cf081