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

Support for CROW_USE_LOCALTIMEZONE for using localtime in logs #368

Merged
merged 1 commit into from
Mar 20, 2022

Conversation

kingster
Copy link
Contributor

@kingster kingster commented Mar 20, 2022

Hi @The-EDev

This changes gmtime_* to localtime_* to use local dates in the logs. Most other HTTP servers usually use local date time by default, hence changing this.

Also wanted your views if this should be configurable or not using a #ifdef? What do you think?

@The-EDev
Copy link
Member

As mentioned here, I think it's best if GMT is default and there's a macro to change it to local time. I just don't know what happened to the work I was supposed to do..

@kingster
Copy link
Contributor Author

kingster commented Mar 20, 2022

Updated the PR with CROW_USE_LOCALTIMEZONE. This will now require an explicit option using #define CROW_USE_LOCALTIMEZONE 1 before including the crow headers

Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

The time in http_server.h must be GMT and not local to the server as per HTTP, please revert the last commit.

@kingster kingster changed the title Use localtime in request logs Support for CROW_USE_LOCALTIMEZONE for using localtime in response/logs Mar 20, 2022
@kingster kingster changed the title Support for CROW_USE_LOCALTIMEZONE for using localtime in response/logs Support for CROW_USE_LOCALTIMEZONE for using localtime in logs Mar 20, 2022
@kingster
Copy link
Contributor Author

The time in http_server.h must be GMT and not local to the server as per HTTP, please revert the last commit.

Done. Sorry, wasn't aware of this.

@kingster kingster force-pushed the feature-localtime branch 2 times, most recently from 727808e to ce432db Compare March 20, 2022 17:20
@The-EDev
Copy link
Member

No worries, and thanks a lot for taking the time to work on improving Crow!

@CrowCpp CrowCpp deleted a comment from crow-clang-format bot Mar 20, 2022
@crow-clang-format
Copy link

--- include/crow/logging.h	(before formatting)
+++ include/crow/logging.h	(after formatting)
@@ -72,17 +72,17 @@
             tm my_tm;
 
 #if defined(_MSC_VER) || defined(__MINGW32__)
-    #ifdef CROW_USE_LOCALTIMEZONE
+#ifdef CROW_USE_LOCALTIMEZONE
             localtime_s(&my_tm, &t);
-    #else
+#else
             gmtime_s(&my_tm, &t);
-    #endif
+#endif
 #else
-    #ifdef CROW_USE_LOCALTIMEZONE
+#ifdef CROW_USE_LOCALTIMEZONE
             localtime_r(&t, &my_tm);
-    #else
+#else
             gmtime_r(&t, &my_tm);
-    #endif
+#endif
 #endif
 
             size_t sz = strftime(date, sizeof(date), "%Y-%m-%d %H:%M:%S", &my_tm);

@CrowCpp CrowCpp deleted a comment from crow-clang-format bot Mar 20, 2022
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

Seems like the only thing left is formatting.

define CROW_USE_LOCALTIMEZONE macro for explict optin for localtimezone in logs
@kingster
Copy link
Contributor Author

Had already fixed the formatting error reported by crow-clang-format. Hoping this time it will pass all the checks.

@kingster
Copy link
Contributor Author

@The-EDev Please check. Github CI reports that all check have passed

@The-EDev The-EDev merged commit 7f2da9b into CrowCpp:master Mar 20, 2022
@kingster kingster deleted the feature-localtime branch March 20, 2022 19:34
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