Adding some smarter defaults for persistence of data #25
Conversation
| this.writeKey = writeKey; | ||
| this.persistentDataPath = persistentDataPath; | ||
| this.persistentDataPath = persistentDataPath ?? Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData); |
There was a problem hiding this comment.
would this work on unity on mobile? since it requires to be Application.persistentDataPath for unity
There was a problem hiding this comment.
Yeah, it worked when I tried it and returned the same value that I got from .Net. I didn't test Unity's Application.persistentDataPath, but from googling it should return the same values. I'm guessing it's a convenience thing?
… determine platform
…, or C# otherwise
| @@ -75,7 +81,19 @@ public class Configuration | |||
| this.apiHost = apiHost; | |||
| this.cdnHost = cdnHost; | |||
| this.exceptionHandler = exceptionHandler; | |||
| this.storageProvider = storageProvider ?? new DefaultStorageProvider(); | |||
|
|
|||
| if (storageProvider != null) | |||
There was a problem hiding this comment.
the more I think the more I feel we probably only need to smart default persist data path. we should always to default to use storage unless the user specifies to use in memory. what do you think?
There was a problem hiding this comment.
Yeah, I'm in favor. More consistent behavior, fewer assumptions and surprises.
| /// </param> | ||
| public Configuration(string writeKey, | ||
| string persistentDataPath, | ||
| string persistentDataPath = null, |
There was a problem hiding this comment.
now that we have a way to provide StorageProvider, I think we can move this parameter to where we create a storage provider. since these two things are related, it makes sense to put them at one place.
… through storage constructor instead of config.
| @@ -33,11 +33,12 @@ public class Configuration | |||
| /// </summary> | |||
| /// <param name="writeKey">the Segment writeKey</param> | |||
| /// <param name="persistentDataPath"> | |||
There was a problem hiding this comment.
need to remove this comment block too
| @@ -65,8 +66,9 @@ public class Configuration | |||
| ICoroutineExceptionHandler exceptionHandler = null, | |||
| IStorageProvider storageProvider = default) | |||
| { | |||
| var platform = SystemInfo.getPlatform(); | |||
There was a problem hiding this comment.
looks like this variable is unused?
| @@ -73,6 +73,12 @@ public interface IStorageProvider | |||
|
|
|||
| public class DefaultStorageProvider : IStorageProvider | |||
| { | |||
| public string PersistentDataPath; | |||
There was a problem hiding this comment.
we should make it a property. public field is discouraged. see here for comparison
Co-authored-by: Wenxi Zeng <[email protected]>
…roperty accessors
No description provided.