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

Datetime format #26

Merged
merged 3 commits into from Oct 31, 2011

Conversation

Projects
None yet
3 participants
@omega
Copy link
Contributor

omega commented Sep 21, 2011

This is somewhat related to issue #24, but not quite. It doesn't give you format strings, but it makes it possible to supply a function that changes the datestrings used via config.

I haven't updated the docs or tests, as the tests failed (I ran them via rake spec) before and after). If that is a requirement for merging, please let me know.

omega added some commits Sep 21, 2011

Make some date related things configurable
You can now supply a config.dateStrings and config.humanMonths.

the config.dateStrings should be a function, which given a timestamp, and an array of month names, should return a hash of date strings, that dateStr then can use to render a proper date giver a interval.

config.humanMonths, if given, replaces the name of months passed to dateStrings.
@thejefflarson

This comment has been minimized.

Copy link
Collaborator

thejefflarson commented Oct 13, 2011

I think you are close, I like the idea of people passing in a function that returns a specialized object, but it needs more documentation.

I'm not so sure about setting config.humanMonths.

Could you remove the typeof checks and rely on _.isFunction too?

@thejefflarson

This comment has been minimized.

Copy link
Collaborator

thejefflarson commented Oct 13, 2011

Also, could you open a ticket for rake spec with exactly what the failure is?

@ashaw

This comment has been minimized.

Copy link
Member

ashaw commented Oct 13, 2011

Re: spec failing, you may not have the closure-compiler gem installed. Run rake check_dependencies to see that you have all the deps.

@omega

This comment has been minimized.

Copy link
Contributor Author

omega commented Oct 14, 2011

I'll see what I can figure out about documenting the changes :)

The reason for allowing changing of humanMonths, was to make writing a Norwegian version of dateStr function easier really.

@thejefflarson

This comment has been minimized.

Copy link
Collaborator

thejefflarson commented Oct 14, 2011

I think it would be better to just override Intervals.dateStrings rather than having two overlapping functions.

@omega

This comment has been minimized.

Copy link
Contributor Author

omega commented Oct 14, 2011

The point of splitting was that being a consumer is easier if you don't have to duplicate the selection of range, and just provide the values

I did run the check deps, and json was missing, installed that and forgot to check again, will do when I have Internet again (on a weekend trip right now :)

On 14. okt. 2011, at 16:25, Jeff Larsonreply@reply.github.com wrote:

I think it would be better to just override Intervals.dateStrings rather than having two overlapping functions.

Reply to this email directly or view it on GitHub:
#26 (comment)

@thejefflarson

This comment has been minimized.

Copy link
Collaborator

thejefflarson commented Oct 14, 2011

Makes sense, can you make the above underscore specific changes and we'll see about merging this in?

@omega

This comment has been minimized.

Copy link
Contributor Author

omega commented Oct 15, 2011

Will do :)

On 14. okt. 2011, at 21:23, Jeff Larsonreply@reply.github.com wrote:

Makes sense, can you make the above underscore specific changes and we'll see about merging this in?

Reply to this email directly or view it on GitHub:
#26 (comment)

@ashaw ashaw merged commit 93eba7b into propublica:master Oct 31, 2011

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