{"action":"created","issue":{"url":"https://api.github.com/repos/spirit-js/spirit/issues/17","repository_url":"https://api.github.com/repos/spirit-js/spirit","labels_url":"https://api.github.com/repos/spirit-js/spirit/issues/17/labels{/name}","comments_url":"https://api.github.com/repos/spirit-js/spirit/issues/17/comments","events_url":"https://api.github.com/repos/spirit-js/spirit/issues/17/events","html_url":"https://github.com/spirit-js/spirit/pull/17","id":257538007,"number":17,"title":"Updated to add pathname and made functions pure, fixed broken tests","user":{"login":"Akkuma","id":342889,"avatar_url":"https://avatars2.githubusercontent.com/u/342889?v=4","gravatar_id":"","url":"https://api.github.com/users/Akkuma","html_url":"https://github.com/Akkuma","followers_url":"https://api.github.com/users/Akkuma/followers","following_url":"https://api.github.com/users/Akkuma/following{/other_user}","gists_url":"https://api.github.com/users/Akkuma/gists{/gist_id}","starred_url":"https://api.github.com/users/Akkuma/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Akkuma/subscriptions","organizations_url":"https://api.github.com/users/Akkuma/orgs","repos_url":"https://api.github.com/users/Akkuma/repos","events_url":"https://api.github.com/users/Akkuma/events{/privacy}","received_events_url":"https://api.github.com/users/Akkuma/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2017-09-13T22:15:55Z","updated_at":"2017-10-23T17:21:42Z","closed_at":"2017-10-20T22:13:17Z","author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/spirit-js/spirit/pulls/17","html_url":"https://github.com/spirit-js/spirit/pull/17","diff_url":"https://github.com/spirit-js/spirit/pull/17.diff","patch_url":"https://github.com/spirit-js/spirit/pull/17.patch"},"body":"`pathname` was added to the request to mimic what a URL contains, which was what `req.url` contained. While I was in there I updated all the functions to be pure and associated tests to use these new pure functions. I also fixed the 5 tests that were failing due to lacking the charset in the content-type."},"comment":{"url":"https://api.github.com/repos/spirit-js/spirit/issues/comments/338732769","html_url":"https://github.com/spirit-js/spirit/pull/17#issuecomment-338732769","issue_url":"https://api.github.com/repos/spirit-js/spirit/issues/17","id":338732769,"user":{"login":"hnry","id":25837,"avatar_url":"https://avatars1.githubusercontent.com/u/25837?v=4","gravatar_id":"","url":"https://api.github.com/users/hnry","html_url":"https://github.com/hnry","followers_url":"https://api.github.com/users/hnry/followers","following_url":"https://api.github.com/users/hnry/following{/other_user}","gists_url":"https://api.github.com/users/hnry/gists{/gist_id}","starred_url":"https://api.github.com/users/hnry/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hnry/subscriptions","organizations_url":"https://api.github.com/users/hnry/orgs","repos_url":"https://api.github.com/users/hnry/repos","events_url":"https://api.github.com/users/hnry/events{/privacy}","received_events_url":"https://api.github.com/users/hnry/received_events","type":"User","site_admin":false},"created_at":"2017-10-23T17:21:42Z","updated_at":"2017-10-23T17:21:42Z","author_association":"COLLABORATOR","body":"There are tests, there are benchmarks. The change will most likely won't affect anything. Or as is now, it won't affect anything.\r\n\r\nSo why not just merge it? Well, when I went to review it and clean up, there were still styling issues. There were 2 broken tests still. And I do take testing very seriously, so backwards compat tests need to be written too to make sure the existing behavior is the same or otherwise noted.\r\n\r\nFor example I believe, the change will make request.query always an object, where as before it can be undefined. Needs to be confirmed, reviewed whether it will affect anything. \r\n\r\nIs it a lot of work? No, but the issues I mentioned need to be taken care of.  Which I will do. And again the other changes I would love to cherry pick it and include, but it's impossible because you grouped all the changes together.\r\n\r\nI take testing very seriously, and just because tests will pass doesn't mean it's perfect. People can make tests pass without knowing what is really going on. For example the tests breaking because of the charset being added to the Content-Type, it's easy to make it pass, but I wanted to know why it broke (because mime package updated, didn't I lock this? I did for major, minor versions, but it changed in a patch version.). \r\n\r\nWriting code is easy, it's the review and accountability and testing that are hard. On the surface it looks like 99% of it won't affect anything. But the 1% needs to be documented and tested to make sure _before_ I just merge a change that isn't a feature or bug fix. If it's a feature or bug fix, then of course I will be more willing to see it merged and spend more effort too. \r\n\r\nTL;DR\r\nIf it ain't broke don't fix it (unless the 'fix' is 100% accounted for). Which it's not in this case. It's not a big deal, but the work I need to put into making this PR ready for merging, I would have done most of the work.\r\n\r\nOther 2 changes, again, I really wish I could you credit via git history, I left a comment noting your contribution. They were fine, just couldn't cherry pick them off your repository. In the future I will ask first. But I was already looking at the source code and felt to just have it get done. If this is an issue you, you can DM me on twitter @nree your github email and I can correct this."}}
{"actor":{"display_login":"hnry"}}