search

inpsyde / Wonolog

Monolog-based logging package for WordPress.
https://inpsyde.github.io/Wonolog/
MIT License
173 stars 17 forks source link

Remove conditions from CI #63

Closed szepeviktor closed 3 years ago

szepeviktor commented 3 years ago

Conditions make code error prone. (and very hard to read)

matrix.composer-options was undefined, it was removed.

gmazzap commented 3 years ago

matrix.composer-options was undefined, it was removed.

Good catch, thank you. That is a leftover from earlier attempts to run the QA also for PHP 8.1/8.2 and used composer-options to skip platform checks for those versions. I removed it in this commit: https://github.com/inpsyde/Wonolog/commit/fc0403182721b041b76f2d1072707802c791c047

Conditions make code error prone. (and very hard to read)

Sorry, but I have to disagree. Your PR adds no functionality still the code moves from 85 lines to 105 lines. And if there's something that IMO makes code error prone is more code (code that doesn't exist can't have bugs).

Moreover that additional 20 lines of code are mostly duplicate code, which would need to be touched everytime I want to change something. So the code will also be harder to maintain.

Regarding to "hard to read", 3 occurrences of something like this: if: ${{ env.USE_COVERAGE == 'yes' }} are IMO not that bad, and surely more immediate to comprehend than the exclude expression on the matrix.

So, I am sorry, but I'm going to close this PR.

This is your 2nd PR I close today, and I'm very sorry about that. I am never happy when I close a PR. I would suggest, next time, before sending a PR to possibly open an issue and discuss changes. That would prevent you doing work that is refused.

szepeviktor commented 3 years ago

You've spent more time ⌛ responding than me removing conditions :)

szepeviktor commented 3 years ago

"CI must not have conditions" is my die-hard policy. Allowing tiny, once-used conditions.