Skip to content
New issue

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

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

Already on GitHub? # to your account

Add a additonal version of Logger::init for initialisation with more meaningful literals #8

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

Conversation

sdicke
Copy link

@sdicke sdicke commented Aug 13, 2019

No description provided.

@juzzlin
Copy link
Owner

juzzlin commented Aug 13, 2019

Hi,

What is the problem this PR solves as there are still only two states just like in the current implementation? I kind of like the approach, but wouldn't like to add another init function that does exactly the same thing..

@@ -66,6 +66,12 @@ class Logger
EpochMicroseconds
};

enum class Append : bool
Copy link
Owner

Choose a reason for hiding this comment

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

A better name for this enum could be something like OpenMode and probably with values Create and Append. In my opinion Append::Append looks a bit weird.

Copy link
Owner

@juzzlin juzzlin left a comment

Choose a reason for hiding this comment

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

Nice work :) If you rename the enum and use switch statement instead of static_cast I will merge this.

@@ -287,6 +287,11 @@ Logger::Logger()
{
}

void Logger::init(std::string filename, Append append)
{
Logger::init(filename, static_cast<bool>(append));
Copy link
Owner

Choose a reason for hiding this comment

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

Here it would be better to use switch statement instead of casting the enum and relying on some specific value.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants