Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renaming endl to RESTendl and other loggers functions #225

Merged
merged 4 commits into from
May 25, 2022
Merged

Conversation

juanangp
Copy link
Member

@juanangp juanangp commented May 24, 2022

juanangp Large: 1276

This PR try to address the misleading use of custom endl and renames the different custom logger functions:

  • Renamed custom endl to RESTendl. Note that we have 2 different ´RESTendl´. endl_t RESTendl inside TRestMetadata and static RESTendl inside TRestStringOuput. However, the implementation has not been changed.
  • Renamed other logger functions
    • info --> RESTInfo
    • debug --> RESTDebug
    • essential --> RESTEssential
    • warning --> RESTWarning
    • fout --> RESTcout
    • ferr --> RESTError
  • Added REST_Verbose_Level enum inside TRestStringOuput class, now verbosity is written as TRestStringOutput::REST_Verbose_Level::REST_XXXX.
  • Added REST_Display_Orientation enum inside TRestStringOuput.h
  • Removal of extern functions from startup.cpp now the custom loggers are defined as static functions inside TRestStringOuput.h
  • Removal of define COLOR_XXX inside TRestStringOuput.h, using constexpr const char* const COLOR_ instead.

Related PR:

…and so on. Adding enums REST_Verbose_Level and REST_Display_Orientation inside TRestStringOutput class.
@jgalan
Copy link
Member

jgalan commented May 24, 2022

what it is doing fout?

Perhaps ferr could just be RESTError?

@juanangp
Copy link
Member Author

what it is doing fout?

I think is standard output with silent verbosity.

Perhaps ferr could just be RESTError?

Yes, perhaps we should have a quorum about this? Currently we have:

  • RESTFout verboseLevel >= silent --> RESTCout ?
  • RESTFerr verboseLevel >= silent --> RESTError ?
  • RESTWarning verboseLevel >= warning
  • RESTEssential verboseLevel >= essential == warning --> RESTLog ?
  • RESTInfo verboseLevel >= info
  • RESTSuccess verboseLevel >= info
  • RESTDebug verboseLevel >= debug
  • RESTExtreme verboseLevel >= extreme

@jgalan
Copy link
Member

jgalan commented May 24, 2022

Ok, I updated RESTFerr because it was kind of obvious when comparing with RESTWarning.

I agree with RESTCout but not sure about RESTLog because this is connected to a verboseLevel="essential" parameter and corresponding output level.

@juanangp
Copy link
Member Author

Ok, I updated RESTFerr because it was kind of obvious when comparing with RESTWarning.

I agree with RESTCout but not sure about RESTLog because this is connected to a verboseLevel="essential" parameter and corresponding output level.

OK, I can change that btw, do you prefer RESTCout or RESTcout without capital letter.

@lobis
Copy link
Member

lobis commented May 24, 2022

I think maybe it would be better to use a namespace for the logger methods i.e. logger::info instead of RESTInfo. What do you think?

Also I think the logger needs a bit of a rework (on a future PR), some of the things I would modify:

Stop using cout like interface for logging messages, use something like https://github.com/fmtlib/fmt instead (probably its alerady packed into root?).

This would allows us, among many things, to define custom formatting for things such as TVector3 or even REST classes so that we can "just print them", reducing significantly the code.

Use https://github.com/gabime/spdlog as the logging library and build a logger on top of it, instead of implementing it from scratch, which has many nice features including https://github.com/gabime/spdlog#backtrace-support

@juanangp
Copy link
Member Author

I think maybe it would be better to use a namespace for the logger methods i.e. logger::info instead of RESTInfo. What do you think?

I think we should use namespaces when the definition of some variables might be misleading, that's why I renamed info to RESTInfo , so now it is clear that this function belongs to REST. Perhaps we should implement a generic namespace REST, then would be trivial to do REST::info, or perhaps REST::logger::info. Since, we are not using many namespaces in the code I don't see the advantage. Also, we should avoid the use of using namespace xxx because it invalidates the first statement, but this is a different topic.

Also I think the logger needs a bit of a rework (on a future PR), some of the things I would modify:

The main issue is that we are using the same implementation for different features, we have the TRestStringOutput logic implemented in TRestMetadata with the verbosity of a particular metadata class using the input in the rml file. Moreover, some errors and warnings are logged inside the TRestMetadata and stored inside the root file. I think is difficult to keep this feature without changing the code design, but we can think of better way to perform the logging.

…enever a custom TRestStringOuput is created inside a class/macro
@juanangp juanangp merged commit c937d49 into master May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants