-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Replace #to_s(:format) with #to_fs #17219
Conversation
rails/rails#43772 rails/rails#44354 Signed-off-by: Takuya Noguchi <takninnovationresearch@gmail.com>
Thank you for opening this PR! We appreciate you! For all pull requests coming from third-party forks we will need to A Forem Team member will review this contribution and get back to |
@@ -41,7 +41,7 @@ xml.rss(:version => "2.0", | |||
xml.item do | |||
xml.title article.title | |||
xml.tag!("dc:creator", user.instance_of?(User) ? user.name : article.user.name) | |||
xml.pubDate article.published_at.to_s(:rfc822) if article.published_at | |||
xml.pubDate article.published_at.to_fs(:rfc822) if article.published_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks exactly like the suggested fix in the deprecation. Thanks!
The xml response being built is used for rss feeds (see https://dev.to/feed for examle), and I don't see any tests around this endpoint (I might not have looked closely enough, but it wasn't in the places I would have expected to find them). We probably want to add a test to confirm whether this is making any changes in behavior.
A minimal spec/requests/articles/articles_feed_spec.rb file covering the response content (not parsing the xml or validating that the rss is readable, only asserting the article is in the feed and the date is formatted the way we expect):
require "rails_helper"
RSpec.describe "ArticlesFeed", type: :request do
let!(:article) { create(:article) }
it "returns an rss feed with published article details" do
get "/feed"
expect(response.body).to include(article.title)
expect(response.body).to include("<pubDate>#{article.published_at.to_fs(:rfc822)}</pubDate>")
end
end
I checked with expecting both of these values in the response body, it showed no difference for me and passed both times (the first does give a deprecation warning)
"<pubDate>#{article.published_at.to_s(:rfc822)}</pubDate>"
"<pubDate>#{article.published_at.to_fs(:rfc822)}</pubDate>"
Signed-off-by: Takuya Noguchi takninnovationresearch@gmail.com
What type of PR is this? (check all applicable)
Description
Replaces
TimeWithZone#to_s(:format)
withTimeWithZone#to_fs
.Related Tickets & Documents
Rails 7 deprecated
TimeWithZone#to_s(:format)
in favor ofTimeWithZone#to_fs
.to_s(format)
in favor ofto_formatted_s(format)
rails/rails#43772from https://app.travis-ci.com/github/forem/forem/jobs/566636747
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
No.