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

Allow to change disk log destination #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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