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

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

Merged
merged 3 commits into from Jan 31, 2019

Conversation

shrektan
Copy link
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

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 added the QA Submitted for QA label Dec 6, 2018
@wch
Copy link
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
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
Copy link
Contributor Author

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

Copy link
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.

None yet

4 participants