Resolve "Verbose option for the logging system" - [merged] #92

Closed rainman110 closed 11 months ago

In GitLab by @rainman110 on Oct 20, 2022, 14:59

Merges 29-verbose-option-for-the-logging-system-2 -> master

This MR adds support for logging of verbose message, as

gtError().verbose() << "This verbose error should must appear";

Depending on the verbosity of the logger, the message will either be logged or not. For more information, look at the tests.

@jensschmeink I tested it also against gtlab core, which works as expected. Note, that the GTLabLogging.dll needs to be copied to GTLab's dll directory, otherwise you'll get an error.

Note, i also fixes many cppcheck issues. If you want to review the verbose() related stuff, just look at the first commit.

Closes #29

In GitLab by @rainman110 on Oct 20, 2022, 15:00

added 1 commit

a7e0383d - Implemented support for verbosity logging

Compare with previous version

In GitLab by @rainman110 on Oct 20, 2022, 15:01

marked this merge request as ready

In GitLab by @rainman110 on Oct 20, 2022, 19:08

@jensschmeink + @marvinoe21 The tests fail here, since the wrong logging library is loaded to LD_LIBRARY_PATH by the rununittests.sh script. It loads the logging lib from devtools instead of using the library that is beeing build.

Any ideas how to fix the in a good way?

In GitLab by @rainman110 on Oct 20, 2022, 19:30

added 1 commit

53c60a2e - Fixed checkstyle warnings

Compare with previous version

In GitLab by @rainman110 on Oct 20, 2022, 19:40

added 1 commit

fc793818 - Fixed checkstyle warnings

Compare with previous version

In GitLab by @rainman110 on Oct 20, 2022, 19:44

added 1 commit

17b05050 - Fixed checkstyle warnings

Compare with previous version

In GitLab by @jensschmeink on Oct 21, 2022, 06:05

I really like the new feature!

In GitLab by @jensschmeink on Oct 21, 2022, 06:06

We need to fix the unittests. The changes themselves look good, so I'll approve after the changes to repair the tests.

In GitLab by @jensschmeink on Oct 21, 2022, 06:18

added 1 commit

c1ac4f5c - use new version of unittests for linux

Compare with previous version

In GitLab by @jensschmeink on Oct 21, 2022, 06:44

added 1 commit

07f4ec93 - fix for unittests by update of skript submodule

Compare with previous version

In GitLab by @jensschmeink on Oct 21, 2022, 06:57

approved this merge request

In GitLab by @rainman110 on Oct 21, 2022, 07:14

requested review from @mariusalexander

In GitLab by @rainman110 on Oct 21, 2022, 07:15

@mariusalexander I'd love to get a review from you since you have done most one the logger recently. :smile:

In GitLab by @mariusalexander on Oct 21, 2022, 07:56

Commented on src/gt_logstream.h line 23

There is also the space method analog to nospace

    inline LogStream &quote() { return static_cast<LogStream&>(QDebug::quote());}
    inline LogStream &space() { return static_cast<LogStream&>(QDebug::space());}

In GitLab by @mariusalexander on Oct 21, 2022, 07:56

Commented on src/gt_logstream.h line 14

what does MEDIUM indicate?

In GitLab by @mariusalexander on Oct 21, 2022, 08:03

I like the new changes. Nice to have unittests as well! Can you explain how the new verbosity level functions? Say the LogLevel is InfoLevel but I use gtDebug().verbose() << "...". When will this be logged?

In GitLab by @mariusalexander on Oct 21, 2022, 08:06

@rainman110 Also Im afraid I did an upsi in the last issue/MR. Have a look at QsLogDisableForThisFile.h, I forgot to add the new gtDebugId etc. macros there. Maybe you could fix this for me in this MR?

In GitLab by @rainman110 on Oct 21, 2022, 08:13

The general idea is to have to verbosity levels (e.g. similar to -v and -vv). -v would be verbose (MEDIUM) and -vv would be even more verbose (everything).

By default, messages that are marked by .verbose() are not displayed at all.

To create a verbose message with the highest level (i.e. would only appear on -vv), just use

gtError().verbose() << ...
gtErrpr().verbose(gt::EVERYTHING) << ...

to write a log message, that appears already on the medium verbosity, write

gtErrpr().verbose(gt::MEDIUM) << ...

To set the middle -v level, call logger.setVerbosity(gt::MEDIUM);

To set the highes level -vv (i.e. to show really all messages), call logger.setVerbosity(gt::EVERYTHING);

Understood?

In GitLab by @rainman110 on Oct 21, 2022, 08:14

added 1 commit

c695a90d - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

In GitLab by @mariusalexander on Oct 21, 2022, 08:14

Commented on src/gt_logstream.h line 15

Consider renaming this enum as it may clutter the gt namespace. For example you may append a suffix to the enum values (also I'd prefer Camel Case enums :stuck_out_tongue: ):

    SilentLog,
    MediumLog,
    LogEverything // or "VerboseLog"

Or alternatively use an enum class?

In GitLab by @mariusalexander on Oct 21, 2022, 08:16

I see, thank you! I like this. This will allow for much more convenient logging

In GitLab by @rainman110 on Oct 21, 2022, 08:57

added 2 commits

1945883f - Simplified tests
c44d36a7 - Applied suggestion from Marius

Compare with previous version

In GitLab by @rainman110 on Oct 21, 2022, 08:58

Commented on src/gt_logstream.h line 15

I was thinking about this too. On the other hand, this will make the logging itself much more inconvenient:

gtError().verbose(gt::Verbosity::SilentLog) << "bla"

In GitLab by @mariusalexander on Oct 21, 2022, 09:02

Commented on src/gt_logstream.h line 15

Thats true...

In GitLab by @rainman110 on Oct 21, 2022, 09:03

Commented on src/gt_logstream.h line 15

SO what to do? Just using camelcase? Or gt::log::silent?

In GitLab by @mariusalexander on Oct 21, 2022, 09:10

Commented on src/gt_logstream.h line 15

Hmm I both like gt::Silent and gt::log::Silent. The latter is a bit more explicit but also a bit more annoying :joy: How about methods like

gtError().silent() << "blup"      // == .verbose(gt::log::Silent)
gtError().medium() << "bli"      // == .verbose(gt::log::Medium)
gtError().everything() << "blap"  // == .verbose(gt::log::Everything)
// or alternatively for the latter
gtError().verbose() << "blip"     // since default param is gt::log::Everything

In GitLab by @rainman110 on Oct 21, 2022, 09:13

Commented on src/gt_logstream.h line 15

good idea, then lets just use verbose and medium.

What about this gtError().verbose(int level) gtError().verbose() gtError().medium()

Everything would be duplicate to verbose(), so skip this. Silent() does not make any sense, as it will never be printed ;)

In GitLab by @mariusalexander on Oct 21, 2022, 09:19

Commented on src/gt_logstream.h line 15

Awesome, I like this. My bad haha

In GitLab by @rainman110 on Oct 21, 2022, 10:28

Commented on src/gt_logstream.h line 23

changed this line in version 9 of the diff

In GitLab by @rainman110 on Oct 21, 2022, 10:28

Commented on src/gt_logstream.h line 14

changed this line in version 9 of the diff

In GitLab by @rainman110 on Oct 21, 2022, 10:28

Commented on src/gt_logstream.h line 15

changed this line in version 9 of the diff

In GitLab by @rainman110 on Oct 21, 2022, 10:28

added 1 commit

60e1d4ac - Improved namespacing and added medium() as a function

Compare with previous version

In GitLab by @rainman110 on Oct 21, 2022, 10:29

@mariusalexander I implemented our ideas. Please check

In GitLab by @mariusalexander on Oct 21, 2022, 10:35

resolved all threads

In GitLab by @mariusalexander on Oct 21, 2022, 10:42

Commented on src/gt_logstream.h line 18

I dont want to be annoying but I'd prefer camel case enums as they are used most often. I'll may add this to our coding guidelines. On a side note, why did you choose these numbers, to have a spectrum from 0-9? :)

    Silent = 0,
    Medium = 5,
    Everything = 9

In GitLab by @rainman110 on Oct 21, 2022, 10:46

Commented on src/gt_logstream.h line 18

I dont want to be annoying but I'd prefer camel case enums as they are used most often.

Okay okay :laughing:

why did you choose these numbers, to have a spectrum from 0-9?

To be potentially able to extend it in future without breaking the API (i.e. to have verbosity levels in between) . This is an old trick from QBasic programming, were you had to write the line number at the beginning of each line. You were always using 10, 20, 30, 40 ... in case you need to add a line in between :smirk:

In GitLab by @rainman110 on Oct 21, 2022, 10:47

Commented on src/gt_logstream.h line 18

changed this line in version 10 of the diff

In GitLab by @rainman110 on Oct 21, 2022, 10:47

added 1 commit

135bb94a - Camelcase for verbosity levels

Compare with previous version

In GitLab by @rainman110 on Oct 21, 2022, 10:52

Commented on src/gt_logstream.h line 18

In GitLab by @mariusalexander on Oct 21, 2022, 11:00

Commented on src/gt_logstream.h line 18

No way, haha thats funny! Crazy... I like the trick :wink:

In GitLab by @mariusalexander on Oct 21, 2022, 11:00

resolved all threads

In GitLab by @mariusalexander on Oct 21, 2022, 11:08

Commented on src/QsLog.h line 94

One last thing, before I'll approve. One may use the macro gtDebugId("FancyTag") to log a message with a custom id. However wouldnt it just log [ancyTa] or am I missing something? You could for example use the reg exp: ^".*?"$ to filter this out

In GitLab by @mariusalexander on Oct 24, 2022, 15:19

Commented on src/QsLog.h line 94

Maybe we should also move the ctor into the .cpp

In GitLab by @rainman110 on Oct 24, 2022, 19:42

Commented on src/QsLog.h line 94

One last thing, before I'll approve. One may use the macro gtDebugId("FancyTag") to log a message with a custom id. However wouldnt it just log [ancyTa] or am I missing something? You could for example use the reg exp: ^".*?"$ to filter this out

Good catch. Indeed gtDebugId("FancyTag") results in [ancyTag].

if we define however GT_MODULE_ID="FancyTag" is will be [FancyTag]. So there is still something wierd.

In GitLab by @rainman110 on Oct 24, 2022, 19:55

Commented on src/QsLog.h line 94

changed this line in version 11 of the diff

In GitLab by @rainman110 on Oct 24, 2022, 19:55

added 1 commit

6a1b98fd - Fixed issue using id-based logging

Compare with previous version

In GitLab by @rainman110 on Oct 24, 2022, 19:55

Commented on src/QsLog.h line 94

This is now fixed!

In GitLab by @rainman110 on Oct 24, 2022, 19:58

Commented on src/QsLog.h line 94

This already contains now the changed semantics of GT_MODULE_ID, which now also needs to be a string, as required by the branch, integrating the moduleid into the metadata.

In GitLab by @mariusalexander on Oct 25, 2022, 07:23

Commented on src/QsLog.h line 94

Ok awesome if this has been fixed

In GitLab by @mariusalexander on Oct 25, 2022, 07:26

resolved all threads

dlr-gtlab / gt-logging

Resolve "Verbose option for the logging system" - [merged] #92