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

Allow date filters to output ordinal days #6773

Merged
merged 4 commits into from Mar 5, 2018

Conversation

Projects
None yet
6 participants
@Ana06
Contributor

Ana06 commented Feb 14, 2018

It is normal to write things like _posted on 12th February 2018" in the blog post. Currently to achieve this, a code similar to this one is needed:

{% assign d = page.date | date: "%-d" %}
{% case d %}
{% when '1' or '21' or '31' %}
{{ d }}st
{% when '2' or '22' %}
{{ d }}nd
{% when '3' or '23' %}
{{ d }}rd
{% else %}{{ d }}th
{% endcase %}
{{ page.date | date: "%B" }}
{{ page.date | date: "%Y" }}

I think the case is common enough to introduce it in Jekyll, by adding some parameters to the date_to_string and date_to_long_string methods.

It supports both UK and US styles. UK is the default.

I also changed @sample_date in one of the tests to cover more cases.

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Feb 14, 2018

I closed #6769 by accident

@ashmaroli I have made the changes we had discussed.

Rubocop complains about the class being to long, should I move date related method to a separate module, similarly to how it is done in url_filters.rb?

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 14, 2018

should I move date related method to a separate module

I don't see the reason to not move this to a separate module. But I'll let the other maintainers make that decision..

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 14, 2018

Are there no native methods in Ruby to handle ordinal dates? What about non-english dates format? e.g, In french you write Jeudi 1er Mars 2018.

@ashmaroli ashmaroli changed the title from Date to Allow date filters to output ordinal days Feb 14, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 14, 2018

Are there no native methods in Ruby to handle ordinal dates?

none that I'm aware of.. But interesting question..

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Feb 14, 2018

@DirtyF

Are there no native methods in Ruby to handle ordinal dates? What about non-english dates format? e.g, In french you write Jeudi 1er Mars 2018

In the Time Ruby class there is nothing. We could suggest them to add it, but it won't happen soon.

I think it is a bad idea to support dates in other languages in Jekyll, but what about extending this methods in your Date plugin? I would be up to help. 😉

So my suggestion is add the ordinal dates in English as it is quite general and used case, and the rest in the plugin. What do you think? 😄

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 14, 2018

@DirtyF DirtyF requested a review from jekyll/core Feb 14, 2018

@DirtyF DirtyF added the enhancement label Feb 14, 2018

@DirtyF DirtyF added this to the v3.8.0 milestone Feb 14, 2018

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Feb 14, 2018

@DirtyF

@Ana06 OK, I guess it makes sense to implement ordinal dates like Rails does:

https://github.com/rails/rails/blob/acbcef6094d61eb8c4820295620d170743a4bd71/activesupport/lib/active_support/inflector/methods.rb#L343-L369

what do you exactly mean? isn't it already done like that? 😕

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 14, 2018

@Ana06 II mean I looked at your implementation and it seems to match the Rail's one, so that would do a nice filter for Jekyll in next minor version

@mattr-

The tests look great! Thanks for making sure everything is covered well. ❤️

There's a few other changes I'd like to see and once those are done, I think this can 🚢

<tr>
<td>
<p class="name"><strong>Date to String in ordinal US style</strong></p>
<p>Format a date to ordinal, UK, short format.</p>

This comment has been minimized.

@mattr-

mattr- Feb 14, 2018

Member

Could you replace UK here with US since it looks like this is for the US format based on the context around it?

@@ -68,22 +68,33 @@ def slugify(input, mode = nil)
end
# Format a date in short format e.g. "27 Jan 2011".
# It allows to use ordinal format as well, in both UK

This comment has been minimized.

@mattr-

mattr- Feb 14, 2018

Member

This comment would read better if it was written thusly:

Ordinal format is also supported, in both the UK and US formats.

Please keep the example dates in this comment when you make the language change. I left them out of the above sentence just to keep that shorter.

return date if date.to_s.empty?
time(date).strftime("%d %B %Y")
def date_to_long_string(date, type = nil, style = nil)
generic_date_to_string(date, "B", type, style)

This comment has been minimized.

@mattr-

mattr- Feb 14, 2018

Member

Could you convert the magic string B into a constant? Same for b as well. Something like:

LONG_MONTH_FORMAT = 'B'
SHORT_MONTH_FORMAT = 'b'

provides much more expressiveness and clarity in the code than the magic string.

This comment has been minimized.

@Ana06

Ana06 Mar 1, 2018

Contributor

I changed it to @ashmaroli suggestion:

instead of passing b and B as parameters, how about passing %b and %B ?

Do you think it is already clear?

I think with that is already clear that is the use of the strftime method and IMO there is no need to add two variables for this. 😄

# month_type: "b", "B"
# type: nil, "ordinal"
# style nil, "US"
def generic_date_to_string(date, month_type, type = nil, style = nil)

This comment has been minimized.

@mattr-

mattr- Feb 14, 2018

Member

Both type and style should default to something other than nil here, IMHO.

This comment has been minimized.

@Ana06

Ana06 Mar 1, 2018

Contributor

nil is equivalent to default. We can give it something more complicate, but the code will work anyway with something different. 🤔

@ashmaroli

Some minor corrections needed.. and a couple of opinions..

</td>
</tr>
<tr>
<tr>

This comment has been minimized.

@ashmaroli

ashmaroli Feb 15, 2018

Member

double <tr> here..

<p class="name"><strong>Date to String in ordinal US style</strong></p>
<p>Format a date to ordinal, UK, short format.</p>
</td>
<td class="align-center">

This comment has been minimized.

@ashmaroli

ashmaroli Feb 15, 2018

Member

Please reorder the table rows here such that information flows gracefully. For example:

Filter Desc. Example
Date to String date_to_string
Date to String in UK Ordinal date_to_string: "ordinal"
Date to String in US Ordinal date_to_string: "ordinal", "US"
Date to Long String date_to_long_string
Date to Long String in UK Ordinal date_to_long_string: "ordinal"
Date to Long String in US Ordinal date_to_long_string: "ordinal", "US"
def date_to_string(date)
time(date).strftime("%d %b %Y")
def date_to_string(date, type = nil, style = nil)
generic_date_to_string(date, "b", type, style)

This comment has been minimized.

@ashmaroli

ashmaroli Feb 15, 2018

Member
  • instead of generic_date_to_string() how about stringify_date() ?
  • instead of passing b and B as parameters, how about passing %b and %B ?

@oe oe referenced this pull request Feb 17, 2018

Closed

Release 3.8.0 #6783

12 of 13 tasks complete
@DirtyF

This comment has been minimized.

Member

DirtyF commented Mar 1, 2018

@Ana06 👋 we're still interested in shipping this for the next minor version, is there anything blocking you from addressing the feedback?

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Mar 1, 2018

@DirtyF

@Ana06 wave we're still interested in shipping this for the next minor version, is there anything blocking you from addressing the feedback?

I was waiting in case there was more feedback, I'll update this now 😉

if type == "ordinal"
day = time.day
ordinal_day = "#{day}#{ordinal(day)}"
return time.strftime("%#{month_type} #{ordinal_day}, %Y") if style == "US"

This comment has been minimized.

@ashmaroli

ashmaroli Mar 1, 2018

Member

you forgot to adjust the string interpolation here and below.. 🙂

- time.strftime("%#{month_type}..
+ time.strftime("#{month_type}..

This comment has been minimized.

@Ana06

Ana06 Mar 1, 2018

Contributor

yes, I was already correcting it 😉

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Mar 1, 2018

@DirtyF I think it should be ready to merge now 😉

@ashmaroli

LGTM! 👍

time.strftime("%d #{month_type} %Y")
end
private

This comment has been minimized.

@Ana06

Ana06 Mar 1, 2018

Contributor

🙈

# month_type: "b", "B"
# type: nil, "ordinal"
# style nil, "US"
# month_type: Notations that evaluate to 'Month' via `Time#strftime` ("%b", "%B")

This comment has been minimized.

@Ana06

Ana06 Mar 1, 2018

Contributor

👍

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Mar 1, 2018

@ashmaroli should I add your correction commit to mines, so that the commit history is clean? 🤔

# type: nil, "ordinal"
# style nil, "US"
# month_type: Notations that evaluate to 'Month' via `Time#strftime` ("%b", "%B")
# type: nil (default) or "ordinal"

This comment has been minimized.

@Ana06

Ana06 Mar 1, 2018

Contributor

to many spaces....

Ana06 added some commits Feb 12, 2018

Add ordinal_date filter
It is normal to write things like _posted on 12th February 2018" in the
blog post. Currently to achieve this, a code similar to this one is
needed:

```
{% assign d = page.date | date: "%-d" %}
{% case d %}
{% when '1' or '21' or '31' %}
{{ d }}st
{% when '2' or '22' %}
{{ d }}nd
{% when '3' or '23' %}
{{ d }}rd
{% else %}{{ d }}th
{% endcase %}
{{ page.date | date: "%B" }}
{{ page.date | date: "%Y" }}
```

I think the case is common enough to introduce it in Jekyll, by adding
some parameters to the `date_to_string` and `date_to_long_string`
methods.

It supports both UK and US styles. UK is the default.
Add test for new format date options
I also changed @sample_date in test to cover more cases. All the dates
tested had the ordinal `th`. Change one of the dates tested to cover a
different case as well.
Move date related method to a separate module
Move date related method to a separate module similary as who it is
done in `url_filters.rb`, as `filters.rb` is getting to long and
Rubocop is not happy with it.
Remove unneeded require "date" in filters.db
The class Date is not used, so there is no need to require.
@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 1, 2018

Feel free to update the branch as you see fit. I only pushed the commit because I didn't want to bother you unnecessarily..

@Ana06

This comment has been minimized.

Contributor

Ana06 commented Mar 1, 2018

@ashmaroli already added. I think I didn't miss anything 😉 I like having clean commit history 💘

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Mar 1, 2018

I think I didn't miss anything

no probs.. my changes seem to have been incorporated nicely.. 👍

@DirtyF DirtyF requested a review from jekyll/core Mar 5, 2018

Changes addressed

@DirtyF

This comment has been minimized.

Member

DirtyF commented Mar 5, 2018

📆 🤙 🔢

Congrats @Ana06 for your first Jekyll contrib! 😍

@jekyllbot: :shipit: +minor

@jekyllbot jekyllbot merged commit ebcd830 into jekyll:master Mar 5, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Ana06 added a commit to Ana06/obs-landing that referenced this pull request May 8, 2018

Update Jekyll! 🎉
This will allow as to use the new date filters to output ordinal days:

jekyll/jekyll#6773

kplattret added a commit to kplattret/jekyll that referenced this pull request Jul 27, 2018

Add version badge for date filters with ordinal
I was getting errors when trying to use the new `date_to_long_string: "ordinal"` parameter on my Jekyll website. I found out that this feature was introduced in Jekyll 3.8.0 (jekyll#6773) and realised that the reason why I couldn't use it was because I had installed Jekyll through the `github_pages` gem, which only supports version 3.7.3 still today. This adds a version badge to the two filters documented here, so that it may help others if they get the same error.

kplattret added a commit to kplattret/jekyll that referenced this pull request Jul 27, 2018

Add version badge for date filters with ordinal
I was getting errors when trying to use the new `date_to_long_string:
"ordinal"` parameter on my Jekyll website. I found out that this feature
was introduced in Jekyll 3.8.0 (jekyll#6773) and realised that
the reason why I couldn't use it was because I had installed Jekyll
through the `github_pages` gem, which only supports version 3.7.3 still
today. This adds a version badge to the two filters documented here, so
that it may help others if they get the same error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment