Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

Proposed fixes for issue #66 only #71

Closed
wants to merge 22 commits into from

Conversation

isocroft
Copy link
Contributor

@isocroft isocroft commented Sep 26, 2018

This PR refers to issue #66 which is currently under discussion. I have made changes to HasBinaryUuid.php in line with the ideas shared on the issue tracker.

@isocroft isocroft changed the title first fixes draft Proposed fixes for issue #65 and issue #66 Sep 26, 2018
@vpratfr
Copy link
Collaborator

vpratfr commented Sep 26, 2018

Hi,

I see you have tests for #65 but no tests for #66. Don't forget to add some.

Additionnally, I know at Spatie they are quite strict about keeping PRs focused on a single topic. Please make 2 separate PRs (for #66 and #65)

@isocroft isocroft changed the title Proposed fixes for issue #65 and issue #66 Proposed fixes for issue #66 only Sep 29, 2018
@isocroft
Copy link
Contributor Author

Hi,

I see you have tests for #65 but no tests for #66. Don't forget to add some.

Additionnally, I know at Spatie they are quite strict about keeping PRs focused on a single topic. Please make 2 separate PRs (for #66 and #65)

Yes, i have made the PR focused on issue #66 only. Also, i have added test

@isocroft isocroft mentioned this pull request Oct 1, 2018
@isocroft
Copy link
Contributor Author

isocroft commented Oct 4, 2018

All done! @vpratfr

@isocroft
Copy link
Contributor Author

Any reason why this PR isn't merged ?

@brendt
Copy link
Contributor

brendt commented Nov 28, 2018

Because I really don't have the time to go through this in depth 😞 @vpratfr does this look ok to you? Can I merge this?

@vpratfr
Copy link
Collaborator

vpratfr commented Nov 28, 2018

Looks ok when reading the code. I had/have no time for real testing though.

On a side note, we have rolled back to standard ids in our current project (lots of complications due to UUIDs when interacting with other packages, in particular Nova & co.). So I am currently not using it anymore :(

@freekmurze
Copy link
Member

@freekmurze freekmurze closed this Dec 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants