JsonSerializable - Githubissues

PUGX / shortid-php

An implementation of ShortId in PHP

MIT License

51 stars 7 forks source link

JsonSerializable #10

Closed EgorGruzdev closed 6 years ago

garak commented 6 years ago

Thanks for your contribution. There are a couple of issues with this PR:

it adds a new method, but it doesn't add a test for it
it contains unrelated features. It would be better to propose a single feeature per PR

EgorGruzdev commented 6 years ago

I wanted to issue as an offer, because I encountered this problem when working with the library. I will try to arrange properly, but I do not have very little experience in this regard.

EgorGruzdev commented 6 years ago

Perhaps this method is not needed here, and each for itself inherits it, if it is needed.

garak commented 6 years ago

The method can be useful, you just need to:

add a test for such method
remove all changes unrelated to new method

thanks

EgorGruzdev commented 6 years ago

Look, did I do that? https://github.com/EgorGruzdev/shortid-php/commit/9b1c81a467bf659f7d6199933d206f0cc48ed75c