Skip to content

Problem with encodeDate()-method - #15

Closed
bkueppers wants to merge 3 commits into
creationix:masterfrom
bkueppers:master
Closed

bkueppers wants to merge 3 commits into
creationix:masterfrom
bkueppers:master

Conversation

@bkueppers

Copy link
Copy Markdown
Contributor

The Github-API seems to have problems with the results that encodeDate produced. Therefore I changed the method to work with the .toISOString()-method, which works fine. The tests failed after that change, so I've commented them out. I do, however, not have enough insight into the project to know whether that change may cause problems at another place.

The Github-API seems to have problems with the results that encodeDate produced. Therefore I changed the method to work with the .toISOString()-method, which works fine. The tests failed after that change, so I've commented them out. I do, however, not have enough insight into the project to know whether that change may cause problems at another place.
@creationix

creationix commented Feb 13, 2017 •

Copy link
Copy Markdown
Owner

The GitHub API has a history of making changes that do things like drop the timezone for internal convenience.

It is a shame that we lose timezones because that is significant to git itself. Your local git hashes won't match what's on github if this information is dropped. I already have code when downloading objects from github where I basically have to guess the timezone to reverse-engineer the given hash.

This will cause issues down the road. Maybe more time can be spent trying to find a format that github accepts and still preserves the timezone? How does the ISO format compare to what I had before?

@creationix

Copy link
Copy Markdown
Owner

And thanks for reporting this and trying to fix it! I'm busy for the next while. Good luck if you decide to try to make it work with github without dropping the time zone.

@bkueppers

bkueppers commented Feb 13, 2017 •

Copy link
Copy Markdown
Contributor Author

For me here (MEC) .toISOString() produces something like:

2017-02-13T21:02:20.621Z

the "Z" at the end indicates, that this is in UCT. Therefore the Date() function of javascript takes care of my timezone by itself, because we would have 2017-02-13T22:02:20.

The official github api v3 documentation says:
"Indicates when this commit was authored (or committed). This is a timestamp in ISO 8601 format: YYYY-MM-DDTHH:MM:SSZ"

Reading this, I think they are expecting a time in UCT. Therefore I guess, my solution should be okay in princple. The only thing that I have to test, is whether I really need to apply the offset or if this would be redundant. I will have to test that tomorrow.

Edit:
I just saw that they are having an example, which states the following:

"date": "2008-07-09T16:13:30+12:00"

So, in the example the "Z" for UTC is omitted, but instead the timezone is explicitly stated, which is also a way of doing it according to ISO 8601.

Having done some more research, I am pretty convinced, that using the Date()-function and then converting to a ISO 8601 representation with .toISOString(), therefore resulting in a time in UTC, should work like intended by github. Especially since converting the local time to UCT and giving the local time and the timezone contains basically the same information. As far as I can see, the UTC is converted by github to the local timezone the browser works in, when viewing a commit in the browser.

@creationix

Copy link
Copy Markdown
Owner

since converting the local time to UCT and giving the local time and the timezone contains basically the same information

The problem with this is the timezone is part of the significant data in git's encoding of commits. If you change the timezone (even if it's the same time), the git hash will be different. Having a different hash locally than what is on github is asking for headaches later on.

@bkueppers

Copy link
Copy Markdown
Contributor Author

Where would I see, that my local hash is different? Is that the hash, that is tested with hasHash() when commiting? So that one would have to be the one, that github assigns to the commit later on?

I've reworked your method and found the bug! So now the timezone information is preserved. The local computed hashes and the hashes generated by github do now match!
@bkueppers

Copy link
Copy Markdown
Contributor Author

Okay, I think I've got it working properly! :-)

@creationix

Copy link
Copy Markdown
Owner

Looking good. Do the disabled tests still fail if you enable them?

@creationix

Copy link
Copy Markdown
Owner

By the way, you can tell if the hash would change if my tests fail. The hash is calculated based on those two numbers, not the string value we send to github.

Tests work now!
@bkueppers

Copy link
Copy Markdown
Contributor Author

First the tests failed, because I forgot the "|0" after the Math.abs(...). I've added that back again, now the tests work. Also the locally computed hash is the same, that a commit has later on on GitHub. So, everything works properly now :-)

@creationix

Copy link
Copy Markdown
Owner

Why did you reformat the entire file? It makes the diff hard to read.

Also why was deleteRef and listRefs removed?

@bkueppers

Copy link
Copy Markdown
Contributor Author

Sorry, something went wrong there... Since it was just a missing Math.abs() in the end, I will delete the Pull Request and my current Fork and fork your repository again. I think this is the easiest way.

@bkueppers bkueppers closed this Feb 15, 2017
@rbren

rbren commented Jan 30, 2018

Copy link
Copy Markdown
Contributor

Did this end up getting merged in a different PR? I'm hitting the same issue - I occasionally get 422 "invalid date-time" responses from GitHub, e.g.

422 Invalid request. 2018-01-30T14:26:20+05:0-18 is not a valid date-time

@rbren rbren mentioned this pull request Jan 30, 2018
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.

3 participants