Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

convertStringToHash #1

Merged
merged 1 commit into from
Mar 24, 2023
Merged

convertStringToHash #1

merged 1 commit into from
Mar 24, 2023

Conversation

marconett
Copy link
Contributor

@marconett marconett commented Mar 24, 2023

Hi,

I compared my implementation (evanw/thumbhash#8) with yours and couldn't find any obvious problems. Yours seems cleaner and more like a complete package, so I'm discarding my implementation.

I added a function convertStringToHash that complements your convertHashToString.

Just two suggestions:

  1. function encode should be renamed to something like RGBAToHash to keep it similar to the complementary function name hashToRGBA.
  2. The lib requires php 8 because of multiple return types in the following line:
    public static function toApproximateAspectRatio(array $hash): float|int

    I know that php 8.1 ist the current actively supported version and 7 is end of life, but a lot of real world projects still rely on older versions.

Thanks for your work on this!

@SRWieZ
Copy link
Owner

SRWieZ commented Mar 24, 2023

Hi !

Thank you for you PR, I appreciate it

Those are great suggestions! I will look into that, I may need to downgrade PestPHP but that's not a major concern.

@SRWieZ SRWieZ merged commit bb4076a into SRWieZ:master Mar 24, 2023
@AyubM
Copy link

AyubM commented Mar 24, 2023

If you are going to rename functions, please make sure to version correctly as I am already using this in production.

@SRWieZ
Copy link
Owner

SRWieZ commented Mar 24, 2023

Oh, already pushed 1.1.0 with naming change.

Sincerely sorry, I honestly thought of it and figured out that no-one would have used it so fast.
Trust me for not doing the same mistake in the future, any breaking changes would imply a major version bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants