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

date/strftime #83

Merged
merged 7 commits into from May 22, 2013
Merged

date/strftime #83

merged 7 commits into from May 22, 2013

Conversation

millermedeiros
Copy link
Member

started date/strftime implementation.

following the tokens present on python, ruby and C

still missing some stuff but basic features are working.. I also added an internal date/i18n_ module to allow setting the locale globally but I guess we need another option to override the default locale as well (maybe 3rd argument).

see #78


it('should support multiple tokens at once', function () {
// iso8601
expect( strftime(date, '%Y-%m-%dT%H:%M:%S%z') ).toEqual( '2013-04-08T09:02:04-0300' );
Copy link
Member Author

Choose a reason for hiding this comment

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

ops, we need to remove the %z otherwise test will fail if timezoneOffset is different..

@millermedeiros
Copy link
Member Author

I'm wondering if we should split each case into its own function (ie. date/getTimezone, date/getCentury, date/toSeconds, date/weekDayName, date/monthNameAbbr, etc..)

It will probably make things easier to reuse and can make code cleaner in some cases (eg. while building a calendar you probably need weekDayName and monthName, strftime(date, '%A') is not very readable..)

@millermedeiros
Copy link
Member Author

implementation is covering all the tokens from http://pubs.opengroup.org/onlinepubs/007908799/xsh/strftime.html besides %V (ISO 8601 week number)

I extracted all the formats that aren't one-liners into separate modules and kept the localization info as a separate file, that way we could potentially create multiple i18n files inside the date package.

strftime is really cryptic, I think we should really implement LDML date format after strftime is done.

@millermedeiros
Copy link
Member Author

I'm almost merging this without the ISO week number support (%V) since I will probably get the implementation wrong (or take a long time to do it right) and I never needed it. This PR is already holding the v0.6 release for too long...

Anyone against an incomplete implementation of strftime landing on v0.6? (with the option to improve it later if needed)

/cc @conradz @MathiasPaumgarten @leocavalcante

@roboshoes
Copy link
Member

Sorry, I've been a little lazy on this PR since it's quite a junk of code to read through. I have no objections at all. A welcomed feature for sure, I was just keeping quiet since I never got around to check out the PR closely. Shame on me.

Anyways 👍 and looking forward to 0.6.0!

@conradz
Copy link
Member

conradz commented May 21, 2013

I would be OK with an incomplete implementation, as long as the incomplete parts (parts implemented in the C strftime and not implemented in this one) are documented. Probably only a few people need the %V.

@millermedeiros
Copy link
Member Author

I rewrote the whole commit history (so it makes more sense) and added docs to all the new methods. I also marked the %V with a <del> (which will render with a strikethrough on the docs) so it should be enough for now.

I think it's good to merge, planning to do it tomorrow and release v0.6 after that.

@roboshoes
Copy link
Member

I will add a de-DE version once it's merged, so I can pull and create e PR.

millermedeiros added a commit that referenced this pull request May 22, 2013
date: strftime, dayOfTheYear, timezoneOffset, timezoneAbbr, weekOfTheYear. improve number/pad.
@millermedeiros millermedeiros merged commit c2f4f42 into mout:master May 22, 2013
@millermedeiros millermedeiros deleted the strftime branch May 22, 2013 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants