Skip to content

Conversation

@samuelkaufman
Copy link

@samuelkaufman samuelkaufman commented Apr 28, 2017

Documented the corresponding issue here #606

@samuelkaufman samuelkaufman changed the title committed failing encode/decode test committed failing encode/decode FormatTimestamp/ParseTimestamp test Apr 28, 2017
@johto
Copy link
Contributor

johto commented Apr 28, 2017

This PR really needs some background information. Why is this a problem?

@samuelkaufman
Copy link
Author

@johto whoops sorry was in progress writing up an issue with the reasoning behind the test case.

@tamird
Copy link
Collaborator

tamird commented Apr 28, 2017

Why don't you include the issue in this PR description (and commit message)? You're making it somewhat more difficult than it needs to be by making it necessary to chase down the issue link (particularly for a future source code reader).

@samuelkaufman
Copy link
Author

@tamird Hey, yeah I had written the pr first, then wrote up the issue, then edited the description of the PR to include the issue number (#606). I didn't go through all the formalities at first because I about 90% expected I was just doing it wrong and it wasn't an issue with lib/pq, so sorry for the sloppiness.

To clarify here as well:

Description

Parsing the output of FormatTimestamp with ParseTimestamp is not possible.

Expected

Based on the documentation:

FormatTimestamp formats t into Postgres' text format for timestamps.
ParseTimestamp parses Postgres' text format.

You would expect Parse to handle the output of Format

Actual

It errors during parse.

I've also included a test that checks that parsed timestamp equals the input timestamp, however I am not sure that is possible and/or right, depending on timezones, rounding, etc.

@tamird
Copy link
Collaborator

tamird commented Apr 28, 2017

Thanks, but I would prefer if you could include the information in the commit message. It can be pretty frustrating, when reading the output of git blame to be directed to a Github issue, requiring an additional context switch.

…lib#606

Description

Parsing the output of FormatTimestamp with ParseTimestamp is not
possible.

* Expected

Based on the documentation:

FormatTimestamp formats t into Postgres' text format for timestamps.
ParseTimestamp parses Postgres' text format.
You would expect Parse to handle the output of Format

* Actual

It errors during parse.

I've also included a test that checks that parsed timestamp equals the
input timestamp, however I am not sure that is possible and/or right,
depending on timezones, rounding, etc.

* Additional Notes

What Postgresql produces from a timestamptz:    2017-04-26 01:09:32.232463+00
What FormatTimestamp produces:                  2017-04-28 18:25:05.070114191Z
@samuelkaufman
Copy link
Author

@tamird got it, I just amended the commit message to have all the info in it as well.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants