Skip to content

need to set the LC_TIME to C to ensure the Date is formatted in English#319

Merged
wch merged 3 commits intorstudio:masterfrom
shrektan:response-date-format
Jan 31, 2019
Merged

need to set the LC_TIME to C to ensure the Date is formatted in English#319
wch merged 3 commits intorstudio:masterfrom
shrektan:response-date-format

Conversation

@shrektan
Copy link
Copy Markdown
Contributor

@shrektan shrektan commented Oct 12, 2018

Hi, the PlumberResponse will attach Date to the header. However, the output of the below line will be different on a different computer (see the example below), which is problematic because normally the non-English date will not be parsed correctly (for example, httr will parse the date as NA)...

With this PR, we can ensure the date is always in English.

https://github.com/trestletech/plumber/blob/8c25b8bfba9774bd422022dc29662ffc17e170bc/R/response.R#L19

The example

date_fmt <- function() {
  cat("Previous: ", format(Sys.time(), "%a, %d %b %Y %X %Z", tz="GMT"))
  cat("\n")
  old_lc_time <- Sys.getlocale("LC_TIME")
  Sys.setlocale("LC_TIME", "C")
  on.exit(Sys.setlocale("LC_TIME", old_lc_time), add = TRUE)
  cat("Now: ", format(Sys.time(), "%a, %d %b %Y %X %Z", tz="GMT"))
}
date_fmt()
#> Previous:  周五, 12 十月 2018 7:04:22 GMT
#> Now:  Fri, 12 Oct 2018 07:04:22 GMT

Created on 2018-10-12 by the reprex package (v0.2.1)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 12, 2018

Codecov Report

Merging #319 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   88.87%   88.98%   +0.11%     
==========================================
  Files          27       27              
  Lines        1177     1189      +12     
==========================================
+ Hits         1046     1058      +12     
  Misses        131      131
Impacted Files Coverage Δ
R/response.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f2e73d...22397cf. Read the comment docs.

@schloerke schloerke added the priority: high Must be fixed before next release label Oct 29, 2018
@schloerke schloerke added this to the v0.4.8 - Next CRAN release milestone Dec 6, 2018
@schloerke schloerke requested review from schloerke and wch December 6, 2018 20:22
@schloerke schloerke added the QA Submitted for QA label Dec 6, 2018
@wch
Copy link
Copy Markdown
Collaborator

wch commented Dec 7, 2018

I think it would be better to implement this without setting the locale. I think that can potentially cause problems if there are multiple threads.

Here's an implementation of the same thing I wrote in C++:

https://github.com/rstudio/httpuv/blob/a830a0d/src/utils.h#L197-L247

Now that I think of it, I could export that function from httpuv.

@wch
Copy link
Copy Markdown
Collaborator

wch commented Dec 12, 2018

Here's an R function that creates a HTTP date string from a POSIXct object. (I wrote this in the process of creating tests for httpuv).

# Given a POSIXct object, return a date string in the format required for a
# HTTP Date header. For example: "Wed, 21 Oct 2015 07:28:00 GMT"
http_date_string <- function(time) {
  weekday_names <- c("Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat")
  weekday_num <- as.integer(strftime(time, format = "%w", tz = "GMT"))
  weekday_name <- weekday_names[weekday_num + 1]

  month_names <- c("Jan", "Feb", "Mar", "Apr", "May", "Jun",
                   "Jul", "Aug", "Sep", "Oct", "Nov", "Dec")
  month_num   <- as.integer(strftime(time, format = "%m", tz = "GMT"))
  month_name <- month_names[month_num]

  strftime(time,
    paste0(weekday_name, ", %d ", month_name, " %Y %H:%M:%S GMT"),
    tz = "GMT"
  )
}


http_date_string(Sys.time())
#> [1] "Wed, 12 Dec 2018 05:54:57 GMT"

@shrektan shrektan force-pushed the response-date-format branch from 5cdc558 to 22397cf Compare December 26, 2018 03:17
@shrektan
Copy link
Copy Markdown
Contributor Author

@wch I've changed the PR based on your code. Thanks.

Copy link
Copy Markdown
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

Looks good!

@wch wch merged commit 677c3d4 into rstudio:master Jan 31, 2019
@schloerke schloerke mentioned this pull request Jan 31, 2019
schloerke added a commit that referenced this pull request Feb 1, 2019
* master:
  need to set the LC_TIME to C to ensure the Date is formatted in English (#319)
  Require httpuv (>= 1.4.5.9000) (#357)
  appveyor pkg cache will bust when DESCRIPTION changes (#370)
  Revamp Swagger Spec to OpenAPI v3 (#365)
schloerke added a commit that referenced this pull request Feb 7, 2019
* master:
  use rtools within appveyor (#381)
  need to set the LC_TIME to C to ensure the Date is formatted in English (#319)
schloerke added a commit that referenced this pull request Feb 7, 2019
* master:
  use rtools within appveyor (#381)
  need to set the LC_TIME to C to ensure the Date is formatted in English (#319)
schloerke added a commit that referenced this pull request Feb 7, 2019
* master:
  use rtools within appveyor (#381)
  need to set the LC_TIME to C to ensure the Date is formatted in English (#319)
  Require httpuv (>= 1.4.5.9000) (#357)
  appveyor pkg cache will bust when DESCRIPTION changes (#370)
  Revamp Swagger Spec to OpenAPI v3 (#365)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: high Must be fixed before next release QA Submitted for QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants