search

ChristianRiesen / base32

Base32 Encoder/Decoder according to RFC 4648
MIT License
131 stars 29 forks source link

Version 2.0 #23

Closed ChristianRiesen closed 3 years ago

ChristianRiesen commented 3 years ago

Trying to get this to a better standard, requiring php 7.4

reedy commented 3 years ago

I note for "us" (MediaWiki/Wikimedia), 7.2 would be appreciated

ChristianRiesen commented 3 years ago

@reedy Thanks for your input. I wasn't aware MediaWiki is using this :) I incorporated all your inputs and got the builds to work with all the versions (7.2 tests with old phpunit, everything else with the newer one). There was no functional need to make it require 7.4 and so I made sure it keeps working and being tested on 7.2 as well. Also getting tested on 8.0 so a move to that should be fine as well.

If you have more inputs, please send them my way, I'll leave this PR open for another day or two before merging and tagging. Not even a BC break happening here, so not a v2 after all :)

reedy commented 3 years ago

Not even a BC break happening here, so not a v2 after all :)

Well... YMMV on whether bumping PHP versions is a semver major change. Different groups argue different ways.

I would say as you're dropping 5.x, it's probably easier/better to just bump the major version; doing so is cheap. Rather than people having something like ^1.3, still being on PHP 5.3 and wondering why 1.4 won't immediately work (though, composer should take of care of that, I think) :)

I'll have another look/review first

ChristianRiesen commented 3 years ago

Thanks again for the inputs @reedy I agree, consistency makes more sense, so since it can add performance, might as well do it. Also added a check to cs fixer for that for the future. Also changed the README. Should be slightly better. Regarding semver, I was of the opinion to make a major version change when changing minimum php version. After a discussion with a co worker I changed my mind. Composer takes care of that, as that is a dependency. The code itself (or it's interfaces really) should dictate a major change or not in the version. Thats at least my thinking here. On yoda, I was not a fan at first, though saw the logical reasoining behind it. I have since starting to use it more seriously seen multiple bugs that could have been prevented by using it, so I'm convinced. It makes the conditions slightly harder to read, but this also points out to me when a condition is too large to properly understand quickly. Should have no influence of the functionality of the library though :)

I'll merge and tag then. More inputs always welcome. Now I need to fix my otp library :)