Implement support for ISO week dates #5

Merged
merged 4 commits into from Nov 21, 2012

Conversation

Projects
None yet
2 participants
@felideon
Contributor

felideon commented Nov 1, 2012

Probably not used very often, but having ISO week date formatting as a part of local-time would be nice to have. It makes sense for certain accounting/financial applications, and for retail/government according to Wikipedia. Personally, I am hacking on a side project where week manipulation is central.

For reference:

http://en.wikipedia.org/wiki/ISO_week_date
http://en.wikipedia.org/wiki/ISO_8601#Week_dates

Allegro's date-time API also has support for this and exposes some date-time-ywd-* functions:
http://www.franz.com/support/documentation/current/doc/date-time.htm#dt-classes-1

Implementation:

The implementation is actually very simple thanks to some existing local-time functions and the following algorithm:
http://en.wikipedia.org/wiki/Talk:ISO_week_date#Algorithms

I started out by including three new values* to decode-timestamp, but this proved (perhaps) to be wrong as it caused the encode-timestamp arglist to become incongruent with the values returned by decode-timestamp , failing a test in the project's test suite. Instead, I wrapped the values in a new m-v-b form around the 'beef' in %construct-timestring.

Formatted-string tests for this feature are also included in this pull request.

Notes:

The three values are mapped to the following keywords for format-timestring:
:ISO-WEEK-YEAR, :ISO-WEEK-NUMBER, :ISO-WEEK-DAY

The first and last of these, which seem to be redundant, are due to the following:

  • The ISO week year can be different than the regular Gregorian calendar year, as an ISO week-numbering year can have 52 or 53 full weeks. http://en.wikipedia.org/wiki/ISO_week_date#Weeks_per_year
  • The weekday number is different than the default (assumed) Sunday=0/Saturday=6 convention. In ISO week date format, Monday=1 and Sunday=7.
Future consideration:

One thing I was on the fence about was whether to expose the ISO week date decoding function in the API. I kept it private for now, as it would be rare to want to have the ISO week number, for example, in isolation from the ISO week year. Conversely, Allegro's API does have this flexibility. For now, it should be easy enough to use format-timestring and split the string to get the individual week date components.

There may also be room to better naming conventions than the ones I chose. :)

Implement ISO week date format
New %timestamp-decode-iso-week function and supporting
FORMAT-TIMESTRING changes, including an +iso-week-format+ constant.
@felideon

This comment has been minimized.

Show comment Hide comment
@felideon

felideon Nov 1, 2012

Owner

This seems to break the congruence between encode-timestamp and decode-timestamp causing one of the tests to fail. It made changes to %construct-timestring easy, but I may have went the wrong way when changing decode-timestamp.

Owner

felideon commented on 7a736c6 Nov 1, 2012

This seems to break the congruence between encode-timestamp and decode-timestamp causing one of the tests to fail. It made changes to %construct-timestring easy, but I may have went the wrong way when changing decode-timestamp.

This comment has been minimized.

Show comment Hide comment
@felideon

felideon Nov 1, 2012

Owner

Fixed the above mentioned issue in b912578 by adding an additional multiple-value-bind in the %construct-timestring function.

@dlowe-net, I was wondering if you could take a look at these changes and let me know what you think. Haven't re-squashed the commits so it's easier to see the progression. (The m-v-b makes the diff harder to parse.)

Owner

felideon replied Nov 1, 2012

Fixed the above mentioned issue in b912578 by adding an additional multiple-value-bind in the %construct-timestring function.

@dlowe-net, I was wondering if you could take a look at these changes and let me know what you think. Haven't re-squashed the commits so it's easier to see the progression. (The m-v-b makes the diff harder to parse.)

This comment has been minimized.

Show comment Hide comment
@dlowe-net

dlowe-net Nov 1, 2012

Hi, Felipe. There's no need to squash the changes. If you send a pull request, it shows all the changes made in the branch and gives us a place to discuss the current state of the branch instead of having the conversation attached to individual commits.

Hi, Felipe. There's no need to squash the changes. If you send a pull request, it shows all the changes made in the branch and gives us a place to discuss the current state of the branch instead of having the conversation attached to individual commits.

This comment has been minimized.

Show comment Hide comment
@felideon

felideon Nov 1, 2012

Owner

Sounds good. Wanted to ping you before coming out of the blue. Thanks.

Owner

felideon replied Nov 1, 2012

Sounds good. Wanted to ping you before coming out of the blue. Thanks.

@dlowe-net

This comment has been minimized.

Show comment Hide comment
@dlowe-net

dlowe-net Nov 4, 2012

Owner

Could you please add the new options to the documentation? Thanks.

Owner

dlowe-net commented Nov 4, 2012

Could you please add the new options to the documentation? Thanks.

@felideon

This comment has been minimized.

Show comment Hide comment
@felideon

felideon Nov 8, 2012

Contributor

@dlowe-net Done. I don't have Tex installed (and the 2GB binary seems to be taking a while to download here) so I couldn't actually test the changes. Let me know if there's anything else. Thanks.

Contributor

felideon commented Nov 8, 2012

@dlowe-net Done. I don't have Tex installed (and the 2GB binary seems to be taking a while to download here) so I couldn't actually test the changes. Let me know if there's anything else. Thanks.

@felideon

This comment has been minimized.

Show comment Hide comment
@felideon

felideon Nov 8, 2012

Contributor

I was able to generate a PDF and it looks fine.

Contributor

felideon commented Nov 8, 2012

I was able to generate a PDF and it looks fine.

@dlowe-net

This comment has been minimized.

Show comment Hide comment
@dlowe-net

dlowe-net Nov 21, 2012

Owner

Looks good. Thanks so much.

Owner

dlowe-net commented Nov 21, 2012

Looks good. Thanks so much.

dlowe-net added a commit that referenced this pull request Nov 21, 2012

Merge pull request #5 from felideon/iso-week-date
Implement support for ISO week dates

@dlowe-net dlowe-net merged commit 2da3c6f into dlowe-net:master Nov 21, 2012

@felideon

This comment has been minimized.

Show comment Hide comment
@felideon

felideon Nov 21, 2012

Contributor

My pleasure!

Contributor

felideon commented Nov 21, 2012

My pleasure!

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