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

DRAFT Implementation of the first version of Sqids #2

Closed
wants to merge 7 commits into from
Closed

DRAFT Implementation of the first version of Sqids #2

wants to merge 7 commits into from

Conversation

roukmoute
Copy link

Fix #1

@roukmoute roukmoute changed the title Implementation of the first version of Sqids DRAFT Implementation of the first version of Sqids Jul 29, 2023
@vinkla
Copy link
Collaborator

vinkla commented Jul 29, 2023

@roukmoute before you spend too much time on this, please see version 0.0.1 @4kimov published yesterday.

@roukmoute roukmoute closed this Jul 29, 2023
@4kimov
Copy link
Member

4kimov commented Jul 29, 2023

@roukmoute I appreciate the changes, but as @vinkla said, your code is a bit behind the main branch.

The main branch code is still a mess at this point, but the majority is there. These are the parts that are left, in case you'd like to create a new PR to push this through the finish line:

  • Replace regular math operations with bcmath/gmp functions
  • Uncomment all tests & make sure they're working
  • I'm rusty on PHP and the code needs lots of cleanup (I'm not up to date on latest PHP conventions, etc)

I'll get to these in a few days unless you finish those before me ;)

@vinkla
Copy link
Collaborator

vinkla commented Jul 29, 2023

I'm rusty on PHP and the code needs lots of cleanup (I'm not up to date on latest PHP conventions, etc)

This can be regarded as done. I've gone ahead added PHP-CS-Fixer workflow and updated the package to follow the latest conventions.

@4kimov
Copy link
Member

4kimov commented Jul 29, 2023

@vinkla Nice 💪

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.

Is it in preparation?
3 participants