Modify file_size help doc string

[j-c-cook]

In hindsight, I'm not sure that the statement, "(or for the case of blf, maximum buffer size before compression and flush to file)" should be in the help doc string for the logger.

The BLFWriter file_size function is a bit more nuanced than is described there. Compression is not accounted for when checking the size of _buffer_size, and therefore if the rollover is caused by the _buffer_size being exceeded, then the resulting file created will have a smaller file size than the file_size requested. If max_container_size << max_bytes then the size of the file will approach the specified max_bytes.

• Provide an explanation for the nuanced file size rollover in the BLFWriter doc string

[codecov]

Codecov Report

Merging #1401 (442d484) into develop (b2b2a80) will decrease coverage by 0.94%.
The diff coverage is n/a.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1401      +/-   ##
===========================================
- Coverage    66.19%   65.25%   -0.95%     
===========================================
  Files           86       81       -5     
  Lines         9068     8767     -301     
===========================================
- Hits          6003     5721     -282     
+ Misses        3065     3046      -19     

[j-c-cook]

@zariiii9003 Do I have this explanation correct of how the file_size function behaves for BLFWriter? Do you think this explanation should be moved from the logger help doc string and into the BLFWriter function? To me it seems (1) perhaps it over complicates the logger help doc string and (2) its not an adequate description of the behavior.

[hardbyte]

👍🏼

[j-c-cook]

@zariiii9003 This would get rid of the nuance. I'm anticipating that you are not going to like the BLFWriter specific code inside of the SizedRotatingLogger constructor, but let me know.

[zariiii9003]

Yes, i don't like this here 😄
Also i do not understand what this does tbh, what's the purpose of this? And why is the default value of kwargs.get('max_container_size', 1) =1?

[j-c-cook]

If max_container_size is not defined then set the max_container_size. If max_container_size is not defined then set the value to be 128 kb unless the file_size is less than 128 kb. In that case set the max_container_size to 10% of the file_size.

I'm reverting the commits because I agree that its not a good location.

[zariiii9003]

Yes, i don't like this here 😄
Also i do not understand what this does tbh, what's the purpose of this? And why is the default value of kwargs.get('max_container_size', 1) =1?

[zariiii9003]

@j-c-cook Can i merge this?

[j-c-cook]

@j-c-cook Can i merge this?

Yes. As long as you think it’s appropriate to have the details contained in the parenthesis. That it does not crowd up the help description printout. I’ve wondered if it’s more than necessary, and if we should just keep it at #1401 (comment).

[zariiii9003]

There are parameters with longer descriptions so i think it's fine.
Alternatively we could disable support for problematic suffixes like gzip and soonish mf4. The original implementation restricted the supported file formats, we could go back to that.

[j-c-cook]

There are parameters with longer descriptions so i think it's fine.

In that case, merge at will.

Alternatively we could disable support for problematic suffixes like gzip and soonish mf4. The original implementation restricted the supported file formats, we could go back to that.

I do agree that this may be useful, but think it should possibly be apart of a separate PR since we may have some details to resolve. For example, I think it would be useful if the dictionary was in the following format, given that some implementations do work with gzip:

supported_writers = {
    BLFWriter: [".blf"],
    CanutilsLogWriter: [".log", ".log.gz"]
}

Note: #1378 is currently a known open issue relating to the topic.