search

pivot-libre / tideman

Implementation of the Tideman ranked pairs algorithm
Apache License 2.0
9 stars 3 forks source link

Tylerharter load #83

Closed carlschroedl closed 6 years ago

carlschroedl commented 6 years ago

I brought the tool you wrote into the rest of the build system.

You should now be able to take advantage of the build's syntax checking, static analysis and style checking by running vendor/bin/phing

Most style deviations can be corrected automatically via vendor/bin/phpcbf src tests.

At some point we can package this as a command line app by adding phar-composer to phing's build.xml. A starting point is this blog post.

Until then the binary can be run via: cd bin php bulk_ballot_test.php -some -args -here

If you get an error about a class not being available, try running phing again to ensure all relevant classes are included in the autoloader that is referenced in bin/bulk_ballot_test.php.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-16.2%) to 82.546% when pulling 4dc0020aedc3eee6ec7d7c0980a12e63ff5d5afd on carlschroedl:tylerharter-load into 319fbf66dacbca38c6ebc3597db3bb744af2b323 on pivot-libre:0.x.

tylerharter commented 6 years ago

Ah, I forgot to clean up this PR. Sorry about distracting you with this one. I already checked in tools/condorcet_checker.php as part of a different PR that I merged.

carlschroedl commented 6 years ago

Sorry Tyler, I do not understand your comment. Could you please take another look at this and perhaps explain the reasoning a different way? I do not understand why this was closed. I think this PR offers largely similar functionality to the recently-merged condorcet_checker.php, but with the added benefit of being compatible with the build system. I propose that we merge this, port any differences in tools/condorcet_checker.php into the classes defined in this PR, and then delete tools/condorcet_checker.php so that we can easily run the auto-generated graph tests while playing nice with the build system.

Does that make sense?

carlschroedl commented 6 years ago

In case it's not clear: I did not write this code from scratch -- it's a Tyler original in slightly different packaging.

carlschroedl commented 6 years ago

Also I don't like the file names I chose, feel free to choose more descriptive names.

carlschroedl commented 6 years ago

I ported in the changes and switched to files names that make more sense.

tylerharter commented 6 years ago

The changes look good! I'm just not very ambitious about merging, so if this were my PR, I would have abandoned once there was the need to merge changes across file names, then start from fresh. Looks like you figured out a way to do it though, so feel free to merge into 0.x.

I wouldn't check in the test cases though (I had just thrown them there to show you).

carlschroedl commented 6 years ago

Ah gotcha, thanks. Yeah in retrospect it might have been faster to do what you mentioned. I can't update the PR now that I'm on the road, so I'm going to merge as-is. Feel free to remove the extra test case files.