Skip to content

Commit

Permalink
Removing "final" restriction so that the class may be mocked
Browse files Browse the repository at this point in the history
  • Loading branch information
Ben Ramsey committed Aug 28, 2012
1 parent edcda15 commit 5215d7e
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/Rhumsaa/Uuid/Uuid.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* @see http://tools.ietf.org/html/rfc4122
* @see http://en.wikipedia.org/wiki/Universally_unique_identifier
*/
final class Uuid
class Uuid
{
/**
* When this namespace is specified, the name string is a fully-qualified domain name.
Expand Down

12 comments on commit 5215d7e

@marijn
Copy link
Contributor

@marijn marijn commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have to ask: why? The immutability is really such a nice trait of the class.

If it is because you want to mock the generators, than my case for a UuidGeneratorInterface and DefaultUuidGenerator are confirmed 😄 I've started working on those together with the generateSequentialRandomUuid generator.

@ramsey
Copy link
Owner

@ramsey ramsey commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem I'm having is with Travis CI and another project that uses this Uuid class. All my builds on Travis CI are failing because this Uuid class requires a 64-bit system, and Travis CI is only 32-bit, so I need to mock the Uuid class so that my tests pass.

I'm happy to make the class immutable again if I can work around the builds failing on Travis CI. Or maybe I just need to take my project off of Travis. :-)

@marijn
Copy link
Contributor

@marijn marijn commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to the other project?

@ramsey
Copy link
Owner

@ramsey ramsey commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I can't. It's a private repository for work.

@marijn
Copy link
Contributor

@marijn marijn commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, you have private beta access, sweet!

Have you considered adding support for 32-bit systems by converting those 64 bit values to strings?
There are a few articles on the matter.

@ramsey
Copy link
Owner

@ramsey ramsey commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about it, but I haven't taken the time yet to see what I would need to do in order to make that happen.

@marijn
Copy link
Contributor

@marijn marijn commented on 5215d7e Aug 28, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could remove the check for the integer size from the constructor to a pre-installation hook in composer. If at any point authors must be warned, it should be at installation time... But I understand it that it wouldn't really be a solution to the underlying problem.

@ramsey
Copy link
Owner

@ramsey ramsey commented on 5215d7e Aug 29, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like Travis CI will be migrating to 64-bit systems in late September to early October, so this may become a non-issue. For now, it's just an annoyance, and while the tests pass fine on a 64-bit system, seeing failed tests on Travis makes me sad. :-)

http://about.travis-ci.org/blog/august-2012-upcoming-ci-environment-updates/

@marijn
Copy link
Contributor

@marijn marijn commented on 5215d7e Jan 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis is currently stil running on 32bit VMs but if you send an email to support@travis-ci.org you can ask to be put on the beta program list 😄

@ramsey
Copy link
Owner

@ramsey ramsey commented on 5215d7e Jan 16, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marijn
Copy link
Contributor

@marijn marijn commented on 5215d7e Jan 16, 2013 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ramsey
Copy link
Owner

@ramsey ramsey commented on 5215d7e Jan 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just pushed the feature-32bit-support. Feel free to review it and let me know if anything's not working or stands out as being awkward or weird. Thanks!

Please sign in to comment.