search

tcpexmachina / remy

403 stars 78 forks source link

Score refactoring #18

Closed pratiksha closed 10 years ago

pratiksha commented 10 years ago

Adding protocol buffers to store scoring "problems" and outcome "answers", plus functions to serialize and parse these.

I'm still working on a minimal example to test this, but thought it would be more useful to submit a pull request in the meantime so I'm not sitting on the code.

anirudhSK commented 10 years ago

Hey Pratiksha,

This pull request looks clean (I can't say for sure without testing), but a few things will help ease the merge process:

  1. You have 5 commits some of which (like "cleaning up") undo previous changes that you made. We ideally want one or two commits that clearly describe the changes to the repository. Typically, you squash these commits into one or two commits after you have tested your changes: http://git-scm.com/book/ch6-4.html#Squashing-Commits The idea is to tell a clean story with the commit messages.
  2. You should not the "using namespace directive" in header files. See: http://www.cplusplus.com/forum/beginner/25538/ for a clear discussion of why.
  3. Also, the formatting changes are definitely important (and we 'll fix them soon), but mixing them in with this commit leads to spurious lines in the diff that have nothing to do with the feature being added. In general, the idea is to keep the diff as minimal as possible so that it eases readability.

If you can fix these, it would be awesome. It 'll make the pull request and associated diff much more readable.

On Fri, Jan 10, 2014 at 2:38 AM, pratiksha notifications@github.com wrote:

Adding protocol buffers to store scoring "problems" and outcome "answers", plus functions to serialize and parse these.

I'm still working on a minimal example to test this, but thought it would be more useful to submit a pull request in the meantime so I'm not sitting

on the code.

You can merge this Pull Request by running

git pull https://github.com/pratiksha/remy score-refactoring

Or view, comment on, or merge it at:

https://github.com/keithw/remy/pull/18 Commit Summary

  • store seed only
  • adding protocol buffers for problem and answer (nothing tested)
  • cleaning up
  • implemented serializing and deserializing; still not tested
  • forgot to serialize configs
  • formatting

File Changes

  • M protobufs/Makefile.amhttps://github.com/keithw/remy/pull/18/files#diff-0(2)
  • A protobufs/answer.protohttps://github.com/keithw/remy/pull/18/files#diff-1(27)
  • A protobufs/problem.protohttps://github.com/keithw/remy/pull/18/files#diff-2(16)
  • M src/evaluator.cchttps://github.com/keithw/remy/pull/18/files#diff-3(141)
  • M src/evaluator.hhhttps://github.com/keithw/remy/pull/18/files#diff-4(26)
  • M src/network.hhhttps://github.com/keithw/remy/pull/18/files#diff-5(20)

Patch Links:

— Reply to this email directly or view it on GitHubhttps://github.com/keithw/remy/pull/18 .

pratiksha commented 10 years ago

I think the revised commits fix these issues; let me know if I missed something.