Thanks! Is this (partly) the same as #1090?

Thanks! Is this (partly) the same as #1090?

Yes. It's basically the same, except I retained the lastWriteTime parameter of the GetFileInfo() method. I'm not convinced that it is correct to use creationTime instead of lastWriteTime in IsDaySwitch() (as in #1090), but I will investigate.

Well we can now unit test it, isn't it?

Thanks for checking :)

I'm not convinced that it is correct to use creationTime instead of lastWriteTime in IsDaySwitch() (as in #1090), but I will investigate.

I think creationTime is better, but please check.

Can you also check the mono error? Thanks!

I think creationTime is better, but please check.

It seems you are right, and I supplemented the tests to enforce this.

And with that, it seems that we never use the lastWriteTime parameter of the GetFileInfo() method. Do you think we should remove this parameter (as in #1090), or keep it for now in case we need it again?

I'm also thinking about refactoring the GetFileInfo() method by replacing all the out parameters with a single out FileInfo parameter (FileInfo being a new class). This should decrease the number of changes when we need to change what file info the GetFileInfo() method returns. What do you think?

It seems you are right, and I supplemented the tests to enforce this.

Thanks!

And with that, it seems that we never use the lastWriteTime parameter of the GetFileInfo() method. Do you think we should remove this parameter (as in #1090), or keep it for now in case we need it again?

Remove it. Less is more (and dead code which isn't clearly dead, is a source of confusion).

I'm also thinking about refactoring the GetFileInfo() method by replacing all the out parameters with a single out FileInfo parameter (FileInfo being a new class). This should decrease the number of changes when we need to change what file info the GetFileInfo() method returns. What do you think?

That's a good proposal. But please don't name it FileInfo. I was already confused before that it wasn't returning a FileInfo instance (the well known BCL FileInfo). Maybe something like FileMetaInfo? (and rename the method)

Current coverage is 73.19%

Merging #1100 into master will decrease coverage by -0.05% as of 3e7e056

@@            master   #1100   diff @@
======================================
  Files          263     264     +1
  Stmts        14972   14928    -44
  Branches      1637    1636     -1
  Methods          0       0       
======================================
- Hit          10966   10927    -39
+ Partial        420     419     -1
+ Missed        3586    3582     -4

Review entire Coverage Diff as of 3e7e056

Uncovered Suggestions

1. +0.09% via ...nternal/AspHelper.cs#206...218
2. +0.09% via ...nternal/AspHelper.cs#188...200
3. +0.09% via ...nternal/AspHelper.cs#170...182
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

Added some small notes.

PS. If you like, please rename some vars to meaningful names. "fi", "ts", those should be removed in the code (in the future)

Thanks! Great job!

PS I only get a notification if you post a comment.

Any idea if this also fixes #307?

Any idea if this also fixes #307?

I don't think so. #307 looks like a threading issue. It does not look like it's due to wrong logic in ShouldAutoArchive(); I think the code is merely not thread-safe, which is not addressed in this PR.

Any idea if this also fixes #307?

I don't think so. #307 looks like a threading issue. It does not look like it's due to wrong logic in ShouldAutoArchive(); I think the code is merely not thread-safe, which is not addressed in this PR.

I think you are right! Thanks for checking!

@bhaeussermann Have you considered making FileCharacteristics a structure? I'm a bit concerned about instantiating a class or two on every write to FileTarget, it may cause significant memory pressure.

Hey @ilya-g! Good to see your here :)

You have a valid point although it's a short living object. @epignosisx what do you think of this? Can we profile the memory pressure on this?