LiamM32 / Eurovision_Condorcet

A program to count votes for the Eurovision Song Contest using a redesigned voting system.
1 stars 2 forks source link

Merge to the last Skeleton updates, adapt it to your code #8

Closed julien-boudry closed 1 year ago

julien-boudry commented 1 year ago

May require a new composer update

LiamM32 commented 1 year ago

I've never done a merge of this nature before. How do I do it?

Would I type ’git merge f003b91’? Would it then insert all these lines which I can then delete?

julien-boudry commented 1 year ago

There are no conflicts. Just use the merge button on Github. Then, on your local environment, make a git pull, and that's it. (and maybe a composer update).

You can go back at any moment with git reset --hard <COMMIT> && git push --force, but everything is tested. You can check on diff log (easily available on this Github PR), I don't remove anything of you.

LiamM32 commented 1 year ago

I just reviewed the differences. They mostly look good. But I must ask; what's the benefit of deleting random-test.php and adding randomTest.php, aside from that the former doesn't work? Is this the minimum change required to make it work?

julien-boudry commented 1 year ago

No, it's not mandatory. But it's better for the development convention to use a testing framework (PHPUnit or PestPHP), it's like a standard and it's more powerful for you. You just have to type composer test, and every test will run, with error formatting etc. You will need to write other tests, to ensure the quality of your implementation.

If you prefer, I revert the change on this file, tell me.

LiamM32 commented 1 year ago

I can try it first. But I don't think I would run this particular test as part of a batch. I would run it on it's own. It's supposed to have an output similar to main.php.

Update: I just downloaded this using git clone on my laptop. I'm normally on my desktop. But it didn't give me bin/. I know I can just copy & paste from Github, but it's strange.

julien-boudry commented 1 year ago

Off course, the test is like a template, to build others later.

Very strange about the bin/, I don't understand too.

LiamM32 commented 1 year ago

Now I see. It's because I was on the master branch. Your new stuff is in the skeleton_update branch.

LiamM32 commented 1 year ago

I have tried it now. While some of these changes are appreciated, I'm hesitant to merge it to master because of two things. One is bin/CondorcetCommandLine.php. It doesn't make sense to try to run this program using the same terminal interface as Condorcet.

The other thing is RandomTest.php. I appreciate that this one actually works, unlike the one I had. But this isn't the way I would want to run it. I would rather just have a .php file I can run with a parameter. I think this approach to testing makes sense for finer details of a program. Just not this particular test. Though I may use the one you wrote as a reference for writing future tests.

julien-boudry commented 1 year ago

I have pushed a new commit restoring the original random_test, keeping a test example. I let you remove the command line program if not needed.

LiamM32 commented 1 year ago

I'm going to do this. I'm just not sure about the 3 merge options, though I'm leaning towards a squash merge.

LiamM32 commented 1 year ago

I merged this, then reverted it, as there are some commits I accidentally added here that shouldn't be on the master branch. But I will definitely merge this if you do the following. git push origin +e5f63f0bdca2f3158bdb0e0a8510e6d2df235e00^:skeleton_update It looks like you may have to do the merge request again though.

julien-boudry commented 1 year ago

I don't understand what you have done. History looks damaged.

LiamM32 commented 1 year ago

Yeah. I deleted that commit. Perhaps I should have just issued a commit to reverse that commit just so you can see what happen. Though I must have chosen to delete it just so that commit 9692500 wouldn't be in the history.

Is there any problem you see besides the latest commit being a day old? Were there any bad consequences to this?

julien-boudry commented 1 year ago

Never modify a history that was shared on a remote. Others can't pull anyway without error. And it's can be hard to reconcile everything, branches no longer share the same tree. Always reverts in that case.

LiamM32 commented 1 year ago

Well then, I suppose you're not going to do the command I wrote above. Too bad that there isn't any good way to get rid of it, as I would prefer to never be reminded of it again.

I am not very good at git, so I frequently make mistakes and scramble to find a way to revert them, and erase any evidence that they ever happened.

Are you having problems with this repository? Is there anything that you recommend I do now?

julien-boudry commented 1 year ago

No problem, all this project is a good practical case to learn ;-)

Let's continue on PR #9