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

FileTarget: Add WriteFooterOnArchivingOnly parameter. #1641

Merged
merged 1 commit into from
Sep 18, 2016

Conversation

bhaeussermann
Copy link
Contributor

@bhaeussermann bhaeussermann commented Sep 14, 2016

Fix #1602


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Sep 14, 2016

Current coverage is 78% (diff: 79%)

Merging #1641 into master will decrease coverage by 2%

@@             master      #1641   diff @@
==========================================
  Files           273        273          
  Lines         16696      16667    -29   
  Methods        2652       2653     +1   
  Messages          0          0          
  Branches       1839       1836     -3   
==========================================
- Hits          13268      12958   -310   
- Misses         3009       3285   +276   
- Partials        419        424     +5   

Sunburst

Powered by Codecov. Last update 43eca98...0fa4e1e

@304NotModified
Copy link
Member

Cool! Thanks !

Not sure about the property name though. What about "onlyFooterOnArchives" ?

@bhaeussermann
Copy link
Contributor Author

Not sure about the property name though. What about "onlyFooterOnArchives" ?

Well, normally the footer is written in the following three cases:

  1. The target is closed,
  2. The file is archived,
  3. We are starting to write to a different file name (without explicit archiving).

If the option claimed to write the footer upon archiving only, that would suggest that the footer would not be written for case (3) above either, which is probably not what we want.

@304NotModified
Copy link
Member

If the option claimed to write the footer upon archiving only, that would suggest that the footer would not be written for case (3) above either, which is probably not what we want.

Well I was think we would like that, because we never known if we would write the file again.

Well, normally the footer is written in the following three cases:

  1. The target is closed,
  2. The file is archived,
  3. We are starting to write to a different file name (without explicit archiving).

With WriteFooterOnTargetClose =false I'm not sure if a footer would be written in case 3.

@bhaeussermann bhaeussermann changed the title FileTarget: Add WriteFooterOnTargetClose parameter. FileTarget: Add WriteFooterOnArchivingOnly parameter. Sep 14, 2016
@bhaeussermann
Copy link
Contributor Author

Well I was think we would like that, because we never known if we would write the file again.

Makes sense. I changed the property to WriteFooterOnArchivingOnly.

@304NotModified
Copy link
Member

👍

@304NotModified 304NotModified removed their assignment Sep 14, 2016
@304NotModified 304NotModified added this to the 4.3.9 milestone Sep 14, 2016
@304NotModified
Copy link
Member

oops merge conflict. Could you fix this?

(PS if you create a branch on the NLog repos, I could also easily fix the merge conflict.)

@304NotModified 304NotModified added file-target enhancement Improvement on existing feature labels Sep 14, 2016
@304NotModified
Copy link
Member

Could you add this to the wiki? Will release it in 4.3.9.

Thanks!

@bhaeussermann
Copy link
Contributor Author

Could you add this to the wiki?

Done.

@304NotModified 304NotModified merged commit b483611 into NLog:master Sep 18, 2016
@304NotModified 304NotModified added the documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) label Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation done all docs done (wiki, api docs, lists on nlog-project.org, xmldocs) enhancement Improvement on existing feature file-target needs documentation on wiki
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants