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 support for environment variables resolution in log file names #570

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

Conversation

Aspirisha
Copy link

@Aspirisha Aspirisha commented Oct 8, 2017

This is a

  • Breaking change
  • New feature
  • Bugfix

I have

The syntax is cmake-like:
E.g., suppose we have ELPP_VAR as environment variable, then configuration file can look like this:

Global:
FORMAT = "%msg"
FILENAME = "matcher_${ELPP_VAR}.log"
ENABLED = true
TO_FILE = true
TO_STANDARD_OUTPUT = true
SUBSECOND_PRECISION = 6
PERFORMANCE_TRACKING = true
MAX_LOG_FILE_SIZE = 2097152 ## 2MB - Comment starts with two hashes (##)
LOG_FLUSH_THRESHOLD = 100 ## Flush after every 100 logs

In case ELPP_VAR is not set, it will be substituted with an empty string.

Copy link
Owner

@abumq abumq left a comment

Choose a reason for hiding this comment

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

This will cause us to lose support for GCC 4.6.4 as regex was implemented in 4.9+ -

see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53631 => Target Milestone section

Can we please do it without depending on regex as it's not worth losing support for two minor versions of GCC just to add little support for environment variables.

@Aspirisha
Copy link
Author

Sure, you are right!

@@ -93,7 +93,7 @@
#else
# define ELPP_OS_MAC 0
#endif
#if (defined(__FreeBSD__) || defined(__FreeBSD_kernel__))
Copy link
Owner

Choose a reason for hiding this comment

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

why remove this?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is some typo, definitely unintended.

@abumq
Copy link
Owner

abumq commented Oct 15, 2017

can you please add test cases as well please? Preferably in TypedConfigurationsTest

@Aspirisha
Copy link
Author

Sure, I will! I just was out of time recently, sorry for not answering :3

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.

None yet

2 participants