Skip to content

Conversation

@renyuneyun
Copy link

@renyuneyun renyuneyun commented Sep 17, 2017

  • Add a builder to construct DiskLogStrategy.
  • Move the default construction of DiskLogStrategy from CsvFormatStrategy to DiskLogStrategy.Builder.
  • Allow to customize log destination.

Copy link

@jefryjacky jefryjacky left a comment

Choose a reason for hiding this comment

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

Can you create builder for set directory and also the set the folder name so it will be more flexible. because sometime user only want to change the folder name only.

@renyuneyun
Copy link
Author

renyuneyun commented Dec 26, 2017

@jefryjacky I don't quite understand your words...
What's the difference between "folder" and "directory" (except they are usually used by windows and unix respectively)?

You could pass a directory name like /sdcard/MY/AWESOME/DIRECTORY to the method and that won't run into a problem.

@jefryjacky
Copy link

I mean the "logger"
directory = diskPath + File.separatorChar + "logger";

the "logger" is hardcoded, I suggest to be able to custom that name.

@renyuneyun
Copy link
Author

@jefryjacky
I think you have misunderstood the meaning. The hardcoded "logger" string is only used in if (directory == null) which means the user (of the library) doesn't provide a customized directory to use.

If changing the subdirectory instead of "logger" is the only thing the coder wants to do, why doesn't him/her pass Environment.getExternalStorageDirectory().getAbsolutePath() + File.separatorChar + "THE_DIRECTORY" to the builder?

@orhanobut
Copy link
Owner

orhanobut commented Apr 1, 2018

Sorry for the late response and thanks for the pull request. I wasn't able to review anything this time period. Apart from a few things, all looks good to me. Would you mind to make the following changes? Then we can merge and release a version for this.

  • Missing tests, that'd be great if you add some tests
  • Support annotation for nullability. Recently I merged a PR which introduced these annotations, which makes easier for kotlin.
  • JavaDoc to cover public API. That helps a lot, especially for generic parameters such as path etc.
  • Update README.md to reflect these changes.

@renyuneyun
Copy link
Author

I have changed some, but with one thing not sure:
How shoud I test the builder? Just confirm the resulting DiskLogStrategy will log?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants