Repository navigation
Conversation
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 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? |
|
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. |
|
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: 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: "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. |
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. |
|
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!
|
Okay, I think I've got it working properly! :-) |
|
Looking good. Do the disabled tests still fail if you enable them? |
|
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!
|
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 :-) |
|
Why did you reformat the entire file? It makes the diff hard to read. Also why was |
|
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. |
|
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. |
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.