Quiet mode for cron-scripts

mokraemer commented 2 years ago

I suggest adding a quite mode, so this script can be added to cron.daily: Adding -q can result to only print a summary, if a table fails and set the exit code !=0

mokraemer commented 2 years ago

I can prepare a diff, if you want...

mokraemer commented 2 years ago

bug5.txt

BenMorel commented 2 years ago

Implemented in 1e88905b0542e8d3b3f92845389be4a01d5d32e3 and 749bb2105ffd73411d894deb8a68aa0e6e501e29.

@mokraemer Could you please test if #2, #3, #4, #5, #6 work for you, before I tag a release?

Please note that I'll review #1 later.

mokraemer commented 2 years ago

I'll check it by tomorrow.

mokraemer commented 2 years ago

may I ask, why you don't like short-options?
what do you think about php >5.4 this is very obsolete, even 7.3 is end of life ;)
while (false !== $database = $statement->fetchColumn()) => you should add () around assignment, every checking tool will complain about probably bad comparison
$echoIfNotQuiet = function($value) use ($options) { why so complicated? just say function doPrint(string $str):void and your fine... what is the advantage in defining it as a dynamic function..
echo ' - ', $table[0], '.', $table[1], "\n"; you used PHP_EOL everywhere else... you should use it here too.

BenMorel commented 2 years ago

may I ask, why you don't like short-options?

It's not that I don't like them, but this project started with long options so I want to keep it consistent. If I later switch to a proper CLI library such as symfony/console, I may support both.

what do you think about php >5.4 this is very obsolete, even 7.3 is end of life ;)

I'm usually more agressive with not supporting older versions in my libraries, but for some reason for this one script I decided to maximize compatibility. I'll probably change this in version 0.2.

while (false !== $database = $statement->fetchColumn()) => you should add () around assignment, every checking tool will complain about probably bad comparison

I'll change this if I later add Psalm to this project and if it complains. At least PhpStorm doesn't.

$echoIfNotQuiet = function($value) use ($options) { why so complicated? just say function doPrint(string $str):void and your fine... what is the advantage in defining it as a dynamic function..

Because I need to access the global variable $options, and don't want to use the global keyword for this. This is a perfect use-case for a Closure.

echo ' - ', $table[0], '.', $table[1], "\n"; you used PHP_EOL everywhere else... you should use it here too.

Good point! Fixed in 43468b253b6f51aa1bac62248c13a521c433fa5b.

Could you please now let me know if all this commits fit your needs, functionally?

mokraemer commented 2 years ago

Sorry I didn't see your reply.

Short-Options: I thought it would be nice to have (at least both), e.g. for common things like quite (which is always q)
while (false !== $database = $statement->fetchColumn()) sometimes it is not very clear what the operator precedense is. At least it makes it clear what is compared and what is assigned ;)
"This is a perfect use-case for a Closure" - I'd say it is the perfect situation where adding things to a class solves so many problems

I'll check yours again, sorry I'm still a bit busy :)

mokraemer commented 2 years ago

Checked. Works as expected.

Tested with MariaDB 10.5
You can add a note, some table-engines (on mariadb) have a table-checksum option which will speedup the test, since checksum is always up-to-date https://mariadb.com/kb/en/aria-storage-engine/

BenMorel commented 2 years ago

"This is a perfect use-case for a Closure" - I'd say it is the perfect situation where adding things to a class solves so many problems

This project started as a simple procedural script, but as it's growing I may decide to refactor it to use classes, external libraries (Symfony console), etc. in the future. But this will require bundling it with phar, so in the meantime I'll try to keep it as simple as possible.

Checked. Works as expected.

Thank you! Let's create a release then.

BenMorel commented 2 years ago

Released as 0.2.0 :tada:

mokraemer commented 2 years ago

NO! Please don't bundle it! It is just good as it is. I was just saying, in case of not using global (which is perfectly fine in a small procedural script) you could pack things in a class an call it. Using e.g. Symfony would make everything just bad.

BenMorel commented 2 years ago

What's the matter with phar and/or Symfony?

mokraemer commented 2 years ago

phar is ok, but why adding dependancies for such a small utility, e.g. like Symfony? Just keep it that small. I've had too many php projects which added a whole bunch of components using "composer". For some simple things like e.g. bigInteger I have sometimes seen 10 incompatible implementations (thanks to composer). So I personally prefer it to keep things small and simple if possible.

BenMorel commented 2 years ago

If you're looking for a solid BigInteger implementation, search no more :)

https://github.com/brick/math

mokraemer commented 2 years ago

Thanks, I'm not - I was running into this, as a I had to implement some greenpass things, and the "composer stuff" pulled ~40 projects. And 5 bigInteger implementations. After cleanup it is 5 projects, same function, less problems... problems of component based development and it looks like implementing bigInteger is a challenge everyone should solve.

Btw. your script works fine (and I can recommend enabling CHECKSUM on tables). I run it every hour at cron without impact on 3 machines. But I must admit I still use my version ;)

BenMorel commented 2 years ago

Glad it helps! No worries, it's open source, you're free to modify it to your needs!

BenMorel / mysql-replication-check

Quiet mode for cron-scripts #5