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

Use moment for better postgres timestamp strings #710

Merged
merged 2 commits into from Jun 18, 2013

Conversation

2 participants
@seth-admittedly
Contributor

seth-admittedly commented Jun 18, 2013

This patch fixes a bug where milliseconds <100 on timestamps were not being correctly zero-padded. For example, March 1, 2013 3:20 p.m. and 53 milliseconds was being stringified as "2013-03-01 15:20:00.53" rather than "2013-03-01 15:20:00.053"

@durango

This comment has been minimized.

Member

durango commented Jun 18, 2013

The build failed because you forgot to update the package.json, BUT I'm incredibly on the fence with including a library such as moment into SequelizeJS (I've thought about this before) the reason why is because momentjs is such a big library to include and the less dependencies the better. @sdepold @mickhansen @janmeier any thoughts on dependencies / momentjs specifically?

@seth-admittedly

This comment has been minimized.

Contributor

seth-admittedly commented Jun 18, 2013

Moment's already in the package.json in the master HEAD https://github.com/sequelize/sequelize/blob/master/package.json#L39. The same adjustment could be done with printf, but the library's already included, presumably because date formatting is such a tricky problem that it's good to outsource it to a dedicated library.

@seth-admittedly

This comment has been minimized.

Contributor

seth-admittedly commented Jun 18, 2013

It looks like some jasmine tests are failing due to the different timezone format that moment uses vs. the previous manual formatting. The moment format better matches postgres' internal format; I'll update the tests and amend the pull.

@durango

This comment has been minimized.

Member

durango commented Jun 18, 2013

Ah fair point, I didn't even realize we had that in our dependencies already. It seems like your tests are fialing because of th enew millisecond addition :)

@durango

This comment has been minimized.

Member

durango commented Jun 18, 2013

Ah you already commented! Yeah update and I'll merge :)

@durango

This comment has been minimized.

Member

durango commented Jun 18, 2013

Actually, let me ask a few people if this should be in the 2.0 branch (since it actually changes an outcome for sequelizejs.. it shouldn't be a problem, but what if someone relied the 0Z for their dates in production? Basically, just trying to get the input of one more person).

durango added a commit that referenced this pull request Jun 18, 2013

Merge pull request #710 from seth-admittedly/postgres-timestamp
Use moment for better postgres timestamp strings

@durango durango merged commit 33be73d into sequelize:master Jun 18, 2013

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