Skip to content

Calendar: add support for ISO8601 basic format#6005

Merged
josevalim merged 1 commit into
elixir-lang:masterfrom
zmackie:iso/basic-enhanced
Apr 21, 2017
Merged

Calendar: add support for ISO8601 basic format#6005
josevalim merged 1 commit into
elixir-lang:masterfrom
zmackie:iso/basic-enhanced

Conversation

@zmackie
Copy link
Copy Markdown

@zmackie zmackie commented Apr 16, 2017 •
edited
Loading

Why?

Currently, Elixir supports the extended format for iso_8601 conversion. This format is good for human readability, but there is also basic format, which omits separator characters, which is often use for machine-machine messages (for example timestamps in http headers). Its easy enough to strip out the separators in a handrolled fashion, but it seems like the language should just support basic directly in addition to the already supported extended format.

This document gives a fairly comprehensive overview of the differences b/w extended and basic (essentially adding or omitting ':' in times and '-' in dates). I would also cite the official ISO 8601 documents but these appear to be behind paywalls.

What?

  • Adds support for basic formatting to DateTime.to_iso8601, Time.to_iso8601, Date.to_iso8601, and NaiveDateTime.to_iso8601
  • Adds corollary support for basic format in Calendar.ISO functions
  • Adds docs, doctest and changes typespec for above
  • Makes extended the default format option for all above

josevalim
josevalim reviewed Apr 16, 2017
View reviewed changes
Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@josevalim josevalim Apr 16, 2017

Choose a reason for hiding this comment

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

No need for the format_mfa indirection. We also don't wrap terms in error messages in Elixir with backticks. Please write the message as:

raise ArgumentError, "DateTime.to_iso8601/2 expects format to be :extended or :basic, got: #{inspect fotmat}"

josevalim
josevalim reviewed Apr 16, 2017
View reviewed changes
Comment thread lib/elixir/lib/calendar/iso.ex Outdated
Copy link
Copy Markdown
Member

@josevalim josevalim Apr 16, 2017

Choose a reason for hiding this comment

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

Wrong indentation.

@josevalim
Copy link
Copy Markdown
Member

josevalim commented Apr 16, 2017

Thank you @Zanadar. We can gladly support the new format but we believe it should also be added to Date, Time and NaiveDateTime as well.

@zmackie zmackie force-pushed the iso/basic-enhanced branch from 007ab6e to 07b63e8 Compare April 17, 2017 00:01
@zmackie zmackie changed the title DateTime.to_iso8601 supports basic format Calendar: add support for ISO8601 basic format Apr 17, 2017
@zmackie zmackie force-pushed the iso/basic-enhanced branch 2 times, most recently from de45ca3 to 65cead2 Compare April 17, 2017 00:15
zmackie
zmackie commented Apr 17, 2017
View reviewed changes
Comment thread lib/elixir/lib/calendar/iso.ex Outdated
Copy link
Copy Markdown
Author

@zmackie zmackie Apr 17, 2017

Choose a reason for hiding this comment

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

done

@zmackie
Copy link
Copy Markdown
Author

zmackie commented Apr 17, 2017

Added the basic format to Date, Time, and NaiveDateTime as well.

lexmag
lexmag suggested changes Apr 17, 2017
View reviewed changes
Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Generally we prefer double quotes over single quotes.
Also let's use backticks around Date.to_iso8601/2 and :basic.

Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017

Choose a reason for hiding this comment

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

I think "human-readability" reads better here.

Copy link
Copy Markdown
Member

@whatyouhide whatyouhide Apr 18, 2017

Choose a reason for hiding this comment

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

I believe "human readability" is the correct way here. http://www.grammarbook.com/punctuation/hyphens.asp

Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

There is double spacing: "readability.<here>It also".

Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017

Choose a reason for hiding this comment

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

Same as for Date docs.

Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017

Choose a reason for hiding this comment

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

Same as for Date docs.

Comment thread lib/elixir/lib/calendar/iso.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017

Choose a reason for hiding this comment

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

I think time_to_string_extended and time_to_string_basic would be better naming for these functions. It reads better that way (since it is not about extended or basic time but about output string), also keeps them grouped by common prefix.

Comment thread lib/elixir/lib/calendar/iso.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 17, 2017

Choose a reason for hiding this comment

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

Same as above.

@zmackie zmackie force-pushed the iso/basic-enhanced branch from 65cead2 to ccd47a5 Compare April 18, 2017 01:26
@zmackie
Copy link
Copy Markdown
Author

zmackie commented Apr 18, 2017

@lexmag Addressed those comments

lexmag
lexmag suggested changes Apr 18, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@lexmag lexmag left a comment

Choose a reason for hiding this comment

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

Besides that last comment, it looks 👍.

Comment thread lib/elixir/lib/calendar.ex Outdated
Copy link
Copy Markdown
Member

@lexmag lexmag Apr 18, 2017

Choose a reason for hiding this comment

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

"dates" should be "times" here. :bowtie:

Copy link
Copy Markdown
Author

@zmackie zmackie Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

🤦‍♂️ fixed that.

@zmackie
Copy link
Copy Markdown
Author

zmackie commented Apr 18, 2017 •
edited
Loading

Looking at "human-readability" vs "human readability" again, I'm inclined to agree with @whatyouhide here that the latter is actually the correct way.

"Readability" is the noun that "human", the adjective, modifies, and thus there is no need for the hyphen. I could be convinced otherwise, but I guess we should have a verdict either way.

@zmackie zmackie force-pushed the iso/basic-enhanced branch from ccd47a5 to af52f97 Compare April 18, 2017 17:56
@whatyouhide
Copy link
Copy Markdown
Member

whatyouhide commented Apr 18, 2017

@Zanadar neither @lexmag nor I are native speakers, so let's trust what the website says and go without hyphen. :)

@zmackie
Calendar support ISO_8601 supports basic format
c0d5c0b 
 - Adds support for `basic` formatting
 - Adds Docs and Doctests for above
 - Makes extended the default format for ISO_8601 formatting
 - DateTime.to_iso_8601 supports basic format
 - Date.to_iso_8601 supports basic format
 - Time.to_iso_8601 supports basic format
 - NaiveDateTime.to_iso_8601 supports basic format
@zmackie zmackie force-pushed the iso/basic-enhanced branch from af52f97 to c0d5c0b Compare April 19, 2017 13:49
@zmackie
Copy link
Copy Markdown
Author

zmackie commented Apr 19, 2017

Hyphens detroyed! Should be good to go.

lexmag
lexmag approved these changes Apr 21, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@lexmag lexmag left a comment

Choose a reason for hiding this comment

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

💛

@josevalim josevalim merged commit cad986d into elixir-lang:master Apr 21, 2017
@josevalim
Copy link
Copy Markdown
Member

josevalim commented Apr 21, 2017

❤️ 💚 💙 💛 💜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@whatyouhide whatyouhide whatyouhide left review comments

@josevalim josevalim josevalim approved these changes

+1 more reviewer

@lexmag lexmag lexmag approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zmackie @josevalim @whatyouhide @lexmag