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

Add example and CustomLogger #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

cksputra
Copy link

@cksputra cksputra commented Aug 7, 2018

Adding example.cpp and CustomLogger.cpp , to compile use "g++ example.cpp CustomLogger.cpp -o example -Wall -Wextra -pedantic -std=c++17 -pthread -Ispdlog_lib -fmax-errors=1 -O3 -march=native"

@ilhamadun
Copy link
Contributor

Please change the following:

  • Rename CustomLogger class to logfmt
  • Move spdlog as a submodule in external/spdlog directory.
  • If I'm not mistaken, spdlog's logger->set_pattern could be called once. I think it's better to move it out of debug, info, warn, error and critical methods.

Copy link
Contributor

@ilhamadun ilhamadun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use snake_case instead of camelCase. E.g. with_field instead of WithField.

logfmt.cpp Outdated

std::string logfmt::Entry::casting(std::string key, std::any val) //casting the value in map into string
{
std::string custom_string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refactor this function to multiple smaller function. Also, maybe it's better to use switch instead of if?

Maybe this would work:

std::string logfmt::Entry::casting(std::string key, std::any val) {
    std::string custom_string;

    if (JSON) {
        custom_string = cast_json(key, value);
    } else {
        custom_string = cast_key_value(key, value);
    }

    return custom_string;
}

std::string logfmt::Entry::cast_json(std::string key, std::any val) {
    switch (val.type()) {
        case typeid(char):
           ...
    }
}

@@ -0,0 +1,48 @@
[2018-08-07 17:18:01.153] [test] [info] INFO of WALRUS and its total animal = "walrus" total = 100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file and add logs/ to .gitignore

@ilhamadun
Copy link
Contributor

Add .clang-format file and fill it with

BasedOnStyle: Google

After that, run clang-format to reformat your code style.

@cksputra
Copy link
Author

cksputra commented Aug 9, 2018

About using switch, trying to change it into switch statement but I think switch in C++ cannot take non - integer or non - enum format so it stills in if format. Please do tell if there is a way to use it or another solution.

@cksputra
Copy link
Author

Updated. Please check the new commit.

@ilhamadun
Copy link
Contributor

I saw a lot of duplication in cast_default and cast_JSON. Try to refactor it to avoid duplication.

.clang-format Outdated
---
Language: Cpp
BasedOnStyle: Google
AccessModifierOffset: -1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you confirm that the clang-format works? I still saw a lot of style inconsistency across the codes. To be safe, start with the first to line without any modification.

Language: Cpp
BasedOnStyle: Google

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the duplication of cast_default and cast_JSON? I think it's only called in process_key function.

Tried it again with clang and nothing change this time so i think it's working, i try it with "clang-format -style=Google logfmt.cpp -i" and also clang-format in atom packages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The codes inside cast_default and cast_json. The part that contains type casting.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Please review the new pull request.

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.

2 participants