Adding minute_file_sink_mt #2727

[mmanoj]

Adding minute_file_sink_mt #2727 to support minute_file_sink to support every 1 min file rotation. minute_file_sink.h.txt

https://github.com/gabime/spdlog/commit/5306b1fd58b84a8b8471c1e0efb6750614a66002

[tt4g]

I think the current rotation implementation is wrong. minute_file_sink rotates at intervals (minutes). However, the daily sink and hourly sink rotate at a specified time (hour or minute). If you are adding sinks that work differently, I think you should change the name of the sinks to make it easier to understand.

[mmanoj]

@tt4g

Only sink name change required? hope the other implemented logic and suggestions are fixed?

[tt4g]

Only sink name change required? hope the other implemented logic and suggestions are fixed?

Did you not develop this sink because you wanted it? If you wanted this sink that rotates on an interval, change the name. If not, please fix it.

[mmanoj]

@tt4g

Yes, This is required as per my project requirements and hope many people like the feature.I change the name to "minute_interval_logger_mt" to make it meaningful.

[gabime]

@mmanoj Please add a unit test for this sink. Something very basic just to verify it compiles right (and perhaps test to validate it throws on invalid input).

[mmanoj]

@mmanoj Please add a unit test for this sink. Something very basic just to verify it compiles right (and perhaps test to validate it throws on invalid input).

@gabime Sure,can you refer some example, I'm checking under test/test_daily_logger

[gabime]

1. Just look at any test in the tests for example. please create a new file in tests with onr or two tests.
2. Also, I think a better name for the sink would be “periodic_file_sink”. This way in the future it could be extended to support any chrono units, not just minutes.
3. Please remove the lime with the email address from the top comments. I try to have same header format for all files

[mmanoj]

1. Working On the test cases files
2. Rename the file and update the sink names as per the advice.Also we can make it seconds as base unit, so we can represent minutes and hours from that. What do you think?
3. Done.

[mmanoj]

https://github.com/mmanoj/spdlog/blob/v1.x/include/spdlog/sinks/periodic_file_sink.h

[gabime]

1. I meant using std::chrono::duration<Rep, Period> interval as for the interval. see https://github.com/gabime/spdlog/blob/57a9fd0841f00e92b478a07fef62636d7be612a8/include/spdlog/spdlog.h#LL85C12-L85C12 for an example.

However, for this to work the generated filename should probably also contain seconds in the name and the units restricted to seconds or higher,(e.g. not accept milliseconds or micros using std::enable_if) so I suggest leaving this for a later stage and start with minutes for this initial version

2. The name should be periodic_file_sink instead of periodic_interval_file_sink.

[mmanoj]

1. I meant using std::chrono::duration<Rep, Period> interval as for the interval. see https://github.com/gabime/spdlog/blob/57a9fd0841f00e92b478a07fef62636d7be612a8/include/spdlog/spdlog.h#LL85C12-L85C12 for an example.

However, for this to work the generated filename should probably also contain seconds in the name and the units restricted to seconds or higher,(e.g. not accept milliseconds or micros using std::enable_if) so I suggest leaving this for a later stage and start with minutes for this initial version

2. The name should be periodic_file_sink instead of periodic_interval_file_sink.

Updated as per advice.Please check and update.Once this clear I will work on the unit test

[gabime]

I see the pr contains 2 sink files? which one is it?

[mmanoj]

I see the pr contains 2 sink files? which one is it?

https://github.com/mmanoj/spdlog/blob/v1.x/include/spdlog/sinks/periodic_file_sink.h

[mmanoj]

I see the pr contains 2 sink files? which one is it?

https://github.com/mmanoj/spdlog/blob/v1.x/include/spdlog/sinks/periodic_file_sink.h

@gabime

Any feedback regarding the PR?

[gabime]

Thanks. Would you add a test?

[mmanoj]

Thanks. Would you add a test?

If current implementation is fine,I will work on it.Please confirm.

[gabime]

The thing is, I cannot know if it even complies without a minimal test. Just add minimal test that includes it and use it in the most basic way.

[mmanoj]

The thing is, I cannot know if it even complies without a minimal test. Just add minimal test that includes it and use it in the most basic way.

sure, will work on this and update you.

[mmanoj]

The thing is, I cannot know if it even complies without a minimal test. Just add minimal test that includes it and use it in the most basic way.

sure, will work on this and update you.

Can you help to understand how spdlog test framework work.I'm not able to run the test cases except example application under spdlog/tests/example directory.

for example how to run test_daily_logger.cpp