Skip to content

Conversation

@Alxandr
Copy link

@Alxandr Alxandr commented Feb 24, 2013

Also, I've checked in the "packages" directory to simplify developement for peoples who want
to extend/fork/fix etc. IMHO a cloned directory is expected to work AS IS, (maybe
having to run a "git submodules init"). When I opened the project it
simply would not compile because of missing references.

@bvanderveen
Copy link
Owner

Hi,

Thanks for you contribution! Couple things before I'm ready to merge:

  • Can you make a separate commit for the the binary check-in?
  • TestRequest.cs seems to have been entirely re-written? If you changed
    whitespace or line endings please make a separate commit for that so we can
    see what changed.
    https://github.com/bvanderveen/httpmachine/pull/8/files#diff-12
    • HttpMessageTests.cs seems to be largely a copy-paste fork from the
      existing test infrastructure and runs the entire parser to test the message
      delegate/handler impl you added. Please isolate that implementation and
      test only that. Also IIRC there is a similar delegate/handler impl in the
      test project—perhaps you could factor that into the main project and test
      it in isolation.

Thanks!
Benjamin

@Alxandr
Copy link
Author

Alxandr commented Feb 26, 2013

Ah, right, I didn't notice that my computer changed line-endings. Will resolve that. The only things I changed in TestRequest was uncomment the "safari" request. I also agree with separating the binary check in, and will do that. Though I'm not entirely sure what you mean by the last point. I figured that the tests should be the same, given that what I wanted to check was that my handler produced the (exact) same output as expected by your library already. The only other way of doing that (as I can think of up front) would be to replicate the functionality of the http-machine inside the test-runner (except you don't actually need to do the parsing, only calling the handler with the appropriate data), however a good reason for leaving the tests as they are is that by doing it the way I am now, I discovered wrongdoings in how I handled the data already. For instance, I didn't know that the OnBody would be called multiple times with parts of the body, so I only saved the ArraySegment given me there initially. However, that ofcause failed a test, and I had to go read your tests to figure out why it was failing.

Also, on a completely different sidenote, I'm the author and host of the NuDoc project, which hosts documentation (at the NuDoc Website). I was wondering if you would allow me to provide documentation for this project. This will require the documentation be added as source-code comments, but will at the same time show the documentation in Visual Studio.

@Alxandr
Copy link
Author

Alxandr commented Feb 26, 2013

There you go. Nice and clean commits.

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.

2 participants