search

nreco / logging

Generic file logger for .NET Core (FileLoggerProvider) with minimal dependencies
MIT License
296 stars 60 forks source link

New setting: Create directory for the log files automatically #9

Closed lmoelleb closed 4 years ago

lmoelleb commented 4 years ago

Considered making it a default option, but that would be a change to the default behavior.

VitaliyMF commented 4 years ago

@lmoelleb I'm ok to accept your PR as is, however maybe "CreateDirectory" option is not needed at all?.. It is possible to check for directory existense in 'OpenFile' and call 'CreateDirectory' only if directory does not exist, and this update in the behavior seems backward-compatible.

lmoelleb commented 4 years ago

The current behavior is to throw an exception if the directory does not exist. Personally I do not need this behavior, but I would consider defaulting to create the directory a change - this is the reason I added the option defaulting to the old behavior.

So as far as I can see, there are three options:

  1. Keep the option as it is now. Guaranties same behavior as the previous version.
  2. Keep the option, but let it default to create the directory. Changes the default behavior, but allows the old behavior with a config change.
  3. Remove the option and simply let it create the directory.

I am OK with all three options. If it was up to me, I would probably go for the last option :)

Based on your preference, I will make a new pull request - I need to at least sort out the merge conflict as Git isn't smart enough to realize it's just two separate tests added the same place.

For checking the directory exists in OpenFile - it is not needed. CreateDirectory will not fail if the directory exists - it just won't do anything.

VitaliyMF commented 4 years ago
  1. Remove the option and simply let it create the directory. I am OK with all three options. If it was up to me, I would probably go for the last option :)

I also vote for variant (3). It's hard to imagine the situation when directory should NOT be created if not exists (this wasn't implemented only because I forgot to make this check!).

Thank you for the contribution :-)

lmoelleb commented 4 years ago

Ok, option 3) it is. PR updated.