search

rubyforgood / loudoun_codes

A ruby on rails app to replace the PCSquared application used for HSPC coding contests.
MIT License
3 stars 10 forks source link

Merge additional SubmissionRunners #115

Closed danielpclark closed 7 years ago

danielpclark commented 7 years ago

The code works, it's clean, and any refactoring that needs to be done may effect multiple runners so if the code base starts getting modified on refactors now the merge master madness will start all over again.

yarmiganosca commented 7 years ago

It's going to be less trouble to converge on a pattern before we start merging stuff in then to merge stuff in because it's there and waiting, and then reconcile later. I'm feeling this right now with your Python and Ruby tests, which are failing because the fake objects you're generating in test are actually from Submission instances. They also don't conform to the pattern I set up in the fixtures directory, so I have to fix that too. We shouldn't merge additional runners until we have a single pattern for writing and testing them. I can include a section the documentation about said patterns once we do so that it's written down.

danielpclark commented 7 years ago

It's going to be less trouble to converge on a pattern before we start merging stuff in then to merge stuff in because it's there and waiting, and then reconcile later.

Wrong. It would be less trouble for those who don't have PRs waiting… which means it's more trouble for me if we converge on the pattern without these merged. Basically you'd be passing off the extra work to me. I have no problem doing broad stroke changes from a single branch, but to do 3 branches at a time along with master IS a pain.

I have no problem about changing them once they're all wrapped neatly together in one source. This is quick and easy and “no trouble” at all for me.

My point is the code is written, they're pretty much identical so everything will be easy to know where to go for changes. I'd prefer it if you didn't defer the trouble to me by breaking my PRs, but rather by merging them and then letting me refactor out the cruft.

danielpclark commented 7 years ago

All I'm asking for is that you trust me. Show me you trust me to do the updates we've discussed. Your code will work much better for a merge once you trust me with it.

yarmiganosca commented 7 years ago

Could you switch your tests to using real model objects instead of fakes before I do?

danielpclark commented 7 years ago

Okay :+1:

danielpclark commented 7 years ago

I think I was tricked.

Could you switch your tests to using real model objects instead of fakes before I do?

This can be read two ways. I agreed when I thought you meant “before you do your PR request” but after I agreed I realized you could mean before you merge my PRs to master.

Clever wording sir. You got me to do a dance through a bunch of branches as I felt obligated to my word. You may not realize this but I have two Crystal branches on my system and two C++ branches so I dance a little harder with the code as I mixed up which branch I was on.