Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Multiple Radius tags for rendering dates is redundant #322

Merged
merged 3 commits into from

4 participants

@SamWhited
Collaborator

Radiant defines two Radius tags for rendering dates, <r:date /> for outputting generic dates using Ruby's strftime and <r:rfc1123 /> which always outputs RFC-1123 formatted dates.

r:date has two attributes, format, and for. The for attribute allows the user to select between the current datetime, the datetime when the page was published, or the datetime when the page was last updated.

r:rfc1123 however, has no such functionality. Which means if the user wants to get the current datetime in RFC-1123 format, they must use the r:date tag, and write a format string for RFC-1123.

The functionality of these two tags feels either incomplete, or redundant.

I see two possible fixes for this.

  1. Allow the r:date tag to accept the string "rfc1123" for the format attribute, and eventually deprecate the r:rfc1123 tag.
  2. Add a for attribute to the r:rfc1123 tag that behaves like the one in r:date.

The problem with the first solution, is that it (sortof) breaks the point of the format string (or at least makes the code a little less elegant). While I can't imagine this really being a problem, it just feels a wrong.

The second solution still requires multiple tags, however, it makes the r:rfc1123 tag much more useful (especially for RSS feeds with often contain <lastBuildDate /> and <pubDate /> tags).

Implementing both solutions (I feel) is just redundant, however, for the purposes of illustration, I threw together a quick patch that does both: https://gist.github.com/1935006

Do people think one or the other solution would be better, or am I creating a problem where one doesn't exist?
I didn't make this issue a pull request, because I definitely think only one of these changes needs to be implemented. I'll actually stick (part of) the patch in my repo and merge it if there's a general consensus that one or the other should be implemented.

@saturnflyer
Owner

Thanks for doing this, Sam.

I like the first option best. I suspect the r:rfc1123 was added as a shortcut so that you wouldn't have to manually handle the format.

We should be able to use an I18n.config entry to set the format as well. So your format="rfc1123" could just be in a locale file.

@SamWhited
Collaborator

Sounds good. I think that's probably the better way to do things too, so if no one else comments or has any suggestions by the time I get home I'll go ahead and update this to a pull request.

Should I move r:rfc1123 into the deprecated_tags.rb file as part of the commit, or is there a specific policy on deprecating the standard tags?

@SamWhited SamWhited was assigned
@saturnflyer
Owner

Yup. Move it to deprecated, but commit to a branch, not master.

Also, raising this question on the dev mailing list is a good idea to get other opinions.

@SamWhited
Collaborator

Will do, thanks.

@jlong
Owner

One other option would be to add an rfc1123="true" attribute to the r:date tag that would override the format option if present.

rfc1123 is actually there to make it easy to write XML feeds as they tend to require this format. I don't think this should be moved into a localization file. It's much better for Radiant to behave the same across all environments. Especially because you can define date formatting using the format option on r:date.

My two cents.

@SamWhited
Collaborator

That might work too; the only reason that I could see for not wanting a separate rfc1123 attribute is because it may be that in the future standards change and we want to include a shortcut for some other commonly used formatting string. In this case, it would seem cluttered to check for multiple attributes (and what happens if the user specifices rfc1123="true" and rfcxyz="true"?).

@johnmuhl
Owner

i prefer option #1; but just get rid of <r:frc1123> and leave <r:date> as is.

<r:frc1123> feels too specifically about blogging to me; so should be part of a blogging extension or the archive extension or something like that.

@saturnflyer
Owner

The option should not move to a locale officially. I'm saying that you can get this feature today with a locale

@SamWhited
Collaborator

I attached a potential fix. What do you think? It's a little weird, since using format="rfc1123" will always be in GMT, whereas using a format string will localize it, but I figured continuing to do it that way will meet existing users expectations.

Also, using a format string will not preserve the time part of the datetime. Should either of these things be changed?

@saturnflyer
Owner

First things first, you have no specs.

Why will using this format attribute always be in GMT? Do you mean UTC? What makes that restriction? Would it not read from the Rails timezone setting or is that how the CGI library behaves?
I don't think that the locale override is a widely known feature, so restricting it with a special argument might not have great impact, but we should consider the impact nonetheless.

Lastly, this might affect our goals with this, I want to introduce an extension that will help us manage unique types of content. Sheets gives us a better way to manage CSS and JS, but we should have something like that to make creating and managing RSS/Atom/robots.txt easier.
If the goal of the original rfc1123 tag was to provide for feeds, a new extension could take care of that responsibility. For that reason I think the rfc tag should be deprecated.

@SamWhited
Collaborator

Specs: Yes, those definitely need to be updated. That was an oversight on my part, which I'll definitely fix before this gets merged anywhere.

Timestamps: The CGI library behaves that way, so I left it for backwards-compatibility with the r:rfc1123_date function. Should be an easy fix if you think it would be better to use the configured time zone. The current implementation of r:date doesn't maintain the time stamp (so it will display the date, but use 00:00:00 for the time portion), so that will probably need to be updated too.

New extension: Sounds like a plan.

@SamWhited SamWhited Added a test for rfc1123 formatted dates using r:date
Also moved the r:rfc1123_date test to the deprecated
tags spec file.
7c1dc48
@SamWhited
Collaborator

Okay just (finally) got around to adding a test for the date tag when format is set to rfc1123, and moved the old r:rfc1123_date test into the deprecated tags spec file.

Still not sure how I feel about doing it this way, but it makes more sense to me than having separate tags.

@saturnflyer saturnflyer merged commit 47097fa into radiant:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2012
  1. @SamWhited
Commits on Mar 12, 2012
  1. @SamWhited

    Added a test for rfc1123 formatted dates using r:date

    SamWhited authored
    Also moved the r:rfc1123_date test to the deprecated
    tags spec file.
  2. @SamWhited
This page is out of date. Refresh to see the latest.
View
9 app/models/deprecated_tags.rb
@@ -30,5 +30,12 @@ module DeprecatedTags
keywords
end
end
+
+ deprecated_tag "rfc1123_date", :deadline => '2.0' do |tag|
+ page = tag.locals.page
+ if date = page.published_at || page.created_at
+ CGI.rfc1123_date(date.to_time)
+ end
+ end
-end
+end
View
32 app/models/standard_tags.rb
@@ -747,10 +747,11 @@ class RequiredAttributeError < StandardError; end
desc %{
Renders the date based on the current page (by default when it was published or created).
- The format attribute uses the same formating codes used by the Ruby @strftime@ function. By
- default it's set to @%A, %B %d, %Y@. The @for@ attribute selects which date to render. Valid
- options are @published_at@, @created_at@, @updated_at@, and @now@. @now@ will render the
- current date/time, regardless of the page.
+ The format attribute uses the same formating codes used by the Ruby @strftime@ function.
+ By default it's set to @%A, %B %d, %Y@. You may also use the string @rfc1123@ as a shortcut
+ for @%a, %d %b %Y %H:%M:%S GMT@ (non-localized). The @for@ attribute selects which date to
+ render. Valid options are @published_at@, @created_at@, @updated_at@, and @now@. @now@ will
+ render the current date/time, regardless of the page.
*Usage:*
@@ -772,9 +773,14 @@ class RequiredAttributeError < StandardError; end
else
page.published_at || page.created_at
end
- @i18n_date_format_keys ||= (I18n.config.backend.send(:translations)[I18n.locale][:date][:formats].keys rescue [])
+ case format
+ when 'rfc1123'
+ CGI.rfc1123_date(date.to_time)
+ else
+ @i18n_date_format_keys ||= (I18n.config.backend.send(:translations)[I18n.locale][:date][:formats].keys rescue [])
format = @i18n_date_format_keys.include?(format.to_sym) ? format.to_sym : format
- I18n.l date, :format => format
+ I18n.l date, :format => format
+ end
end
desc %{
@@ -972,20 +978,6 @@ def snippet_cache(name)
end
desc %{
- Outputs the published date using the format mandated by RFC 1123. (Ideal for RSS feeds.)
-
- *Usage:*
-
- <pre><code><r:rfc1123_date /></code></pre>
- }
- tag "rfc1123_date" do |tag|
- page = tag.locals.page
- if date = page.published_at || page.created_at
- CGI.rfc1123_date(date.to_time)
- end
- end
-
- desc %{
Renders a list of links specified in the @paths@ attribute according to three
states:
View
8 spec/models/deprecated_tags_spec.rb
@@ -105,6 +105,12 @@
end
end
end
+
+ describe "<r:rfc1123_date>" do
+ it 'should render an RFC1123-compatible date' do
+ page(:dated).should render('<r:rfc1123_date />').as('Wed, 11 Jan 2006 00:00:00 GMT')
+ end
+ end
describe "<r:navigation>" do
it "should render with deprecated url attribute" do
@@ -121,4 +127,4 @@
end
end
end
-end
+end
View
8 spec/models/standard_tags_spec.rb
@@ -613,6 +613,10 @@
page.should render('<r:date format="%d %b %Y" />').as('11 Jan 2006')
end
+ it "should render an RFC1123-compatible date if the 'format' attribute is set to 'rfc1123'" do
+ page.should render('<r:date format="rfc1123" />').as('Wed, 11 Jan 2006 00:00:00 GMT')
+ end
+
it "should format the published date according to localized format" do
page.should render('<r:date format="short" />').as(I18n.l(page.published_at, :format => :short))
end
@@ -881,10 +885,6 @@
page.should render('<r:escape_html><strong>a bold move</strong></r:escape_html>').as('&lt;strong&gt;a bold move&lt;/strong&gt;')
end
- it '<r:rfc1123_date> should render an RFC1123-compatible date' do
- page(:dated).should render('<r:rfc1123_date />').as('Wed, 11 Jan 2006 00:00:00 GMT')
- end
-
describe "<r:breadcrumbs>" do
it "should render a series of breadcrumb links separated by &gt;" do
expected = %{<a href="/">Home</a> &gt; <a href="/parent/">Parent</a> &gt; <a href="/parent/child/">Child</a> &gt; <a href="/parent/child/grandchild/">Grandchild</a> &gt; Great Grandchild}
Something went wrong with that request. Please try again.