Resolve "Forward time and logging id to Logging Destination" - [merged] #97

Closed rainman110 closed 10 months ago

In GitLab by @mariusalexander on Nov 17, 2022, 13:56

Merges 34-forward-time-and-logging-id-to-logging-destination -> master

Closes #34

In GitLab by @mariusalexander on Nov 17, 2022, 15:11

added 1 commit

f7aabc28 - refactored write method, will recieve id, time, level and message unformatted;...

Compare with previous version

In GitLab by @mariusalexander on Nov 17, 2022, 15:18

added 1 commit

d4de2d73 - cleanup

Compare with previous version

In GitLab by @mariusalexander on Nov 17, 2022, 15:35

added 1 commit

f1a02e24 - fix

Compare with previous version

In GitLab by @mariusalexander on Nov 17, 2022, 15:48

added 1 commit

c8f0f672 - replaced time formating function

Compare with previous version

In GitLab by @mariusalexander on Nov 17, 2022, 15:53

added 1 commit

18eded0d - cleanup

Compare with previous version

In GitLab by @mariusalexander on Nov 18, 2022, 09:17

added 1 commit

67de9217 - some refactoring of the formatter, added dedicated unittests

Compare with previous version

In GitLab by @mariusalexander on Nov 18, 2022, 15:29

added 1 commit

ca9fa400 - removed string escapement

Compare with previous version

In GitLab by @mariusalexander on Nov 18, 2022, 15:45

I have changed the signature of the Destination::write method to recieve an unformatted message, time, level and the id.

Then I have experimented a bit with a generalized formatter, that accepts a format string (e.g. /L /M /I for <Level> <Message> <id>, see test_logformatter for more details). I have wrapped this in another destination class FormattedDestination which is used by the file and output destination, however the Formatter can be used standalone as well. However I see two issues with the current formatter implementation:

If the id string is empty and [/I] or something similar is used it would simply log [] (idealy it would log nothing)
One cannot escape the / currently.

implementing this is correctly probably quite challenging. What do you think @rainman110? Should we stick with the generalized formatter approach using a format string or should I simply implement a "formatter interface" and a default formatter that would log our messages like they used to (i.e. <level> ([<id>]) [<time>] <message>) - if you can follow me?

Btw I have also experimented with an "informativeWrite" method, that would only log the message itself without an id, time or level etc, but Im not sure if we should keep this approach either.

In GitLab by @rainman110 on Nov 21, 2022, 08:03

Without thinking too much about this, I would favour a more standard approach on, how the format string should look like.

I can see 3 options:

format("['Reference to deleted milestone 1'] ['Reference to deleted milestone 2'] - 'Reference to deleted milestone 3'", date, id, msg); (aka Qt style formatting) - Order dependent!
format["[%s] [%s] - %s", date.toString, id, msg) (c-style formatting) - Order dependent!
format("[%DATE] [%ID] - %MESSAGE", date, id msg) (custom formatting) - Order independent!

In GitLab by @rainman110 on Nov 21, 2022, 08:11

Also maybe another Idea. Instead of blowing up the interface of "message", we could add only 1 additional argument: LogMeta

It will currently be

struct LogMeta { std::string logId; std::time_t time; };

The message API then will be:

void message(LogLevel, std::string msg, LogMeta)

We can expand this later on without blowing up againt the function signature

In GitLab by @mariusalexander on Nov 21, 2022, 08:44

Good idea, why haven't I thought of this :joy:

In GitLab by @mariusalexander on Nov 21, 2022, 13:50

added 1 commit

29b8120c - reworked format function, now using qt arg like approach

Compare with previous version

In GitLab by @mariusalexander on Nov 21, 2022, 13:51

I have updated the signature.

In GitLab by @mariusalexander on Nov 21, 2022, 13:53

Good idea, I have implemented a standalone function and opted for the first option - order dependent qt like arguments list.

In GitLab by @mariusalexander on Nov 21, 2022, 13:55

The struct could hold a bool/flag too, which may indicate that a message is meant to be simply informative (i.e. dont format the string, only print the mesage). That way we would not have two virtual "write" methods in the Destination. What do you think?

In GitLab by @mariusalexander on Nov 21, 2022, 15:49

added 1 commit

b4e9b6e4 - refactored formatter interface, cleanup

Compare with previous version

In GitLab by @rainman110 on Nov 21, 2022, 16:14

Commented on src/gt_loglevel.h line 42

Do you want to combine LogLevels, or why do you use bits here?

In GitLab by @rainman110 on Nov 21, 2022, 16:15

Commented on src/gt_loglevel.cpp line 14

you could use constexpr here, right?

In GitLab by @rainman110 on Nov 21, 2022, 16:21

Commented on src/gt_logdestfunctor.h line 71

Does C++ 14 suppport auto return types? If unsure, better return a typed Type.

In GitLab by @rainman110 on Nov 21, 2022, 16:23

Commented on src/gt_logdestfile.h line 182

I think we should return a concrete type here (i.e. shared_ptr), to make the semantics of the function clear.

By the way, do we really need a shared_ptr or would be a unique_ptr sufficient?

In GitLab by @rainman110 on Nov 21, 2022, 16:25

Commented on src/gt_logdestconsole.h line 64

Same as in the previous discussion.

In GitLab by @rainman110 on Nov 21, 2022, 16:27

Commented on tests/unittests/test_logdestfunctor.cpp line 2

Why did you remove these tests?

In GitLab by @rainman110 on Nov 21, 2022, 16:29

Commented on src/gt_logformatter.h line 154

Why does the Formatter do the filtering? This should be the responsibilty of the logger, not of the Formatter!

Imho, this violates Single Responsibility Principle.

In GitLab by @rainman110 on Nov 21, 2022, 16:32

Commented on tests/unittests/test_types.cpp line 3

Why did you remove so many tests? There were also tests for standard types in here? (std::vector, std::list, std::map, std::multimap)

In GitLab by @rainman110 on Nov 21, 2022, 16:34

Commented on tests/unittests/test_verbosity.cpp line 72

What is this test testing?

In GitLab by @mariusalexander on Nov 22, 2022, 08:22

Commented on src/gt_loglevel.h line 42

Exactly. I've used this for the filter in the Formatter class.

In GitLab by @mariusalexander on Nov 22, 2022, 08:23

Commented on src/gt_logdestfunctor.h line 71

C++14 does afaik. But your right

In GitLab by @mariusalexander on Nov 22, 2022, 08:25

Commented on tests/unittests/test_types.cpp line 3

I have split them in seperate test files, one for std only and for qt only. However std::vector, std::list, std::map, std::multimap are currently not supported by our stream class but by QDebug

In GitLab by @mariusalexander on Nov 22, 2022, 08:25

Commented on tests/unittests/test_verbosity.cpp line 72

A remnant, I'll remove it :+1:

In GitLab by @mariusalexander on Nov 22, 2022, 08:27

Commented on src/gt_logformatter.h line 154

What if a destination would like to log only certain levels? I thought this could be a neat feature. Maybe I'll make this a standalone function?

In GitLab by @mariusalexander on Nov 22, 2022, 08:28

Commented on tests/unittests/test_logdestfunctor.cpp line 2

They were duplicates INMO / didn't test something new (see test_logdest.cpp)

In GitLab by @rainman110 on Nov 22, 2022, 08:38

Commented on src/gt_logformatter.h line 154

The destination can itself implement the filtering for its own. Normally, this is the responsibility of the logger class itself to filter out the logging messages. Still, I don't think that this belongs into the Formatter.

In GitLab by @rainman110 on Nov 22, 2022, 08:39

Commented on tests/unittests/test_types.cpp line 3

Okay, got it.