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

Update code to work with php5.3 + add unit test #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rande
Copy link

@rande rande commented Mar 17, 2012

No description provided.

@chregu
Copy link
Owner

chregu commented Mar 18, 2012

Could you please seperate White Space only, test additions and code changes in single PRs or at least single commits?

And please explain a little bit more, what exactly you did? You "just" introduced a namespace and tests? (thanks anyway for the effort)

I'm ususally not a big fan of introducing namespaces just for the sake of it (if it's a single class library which didn't have them before), but I'll think about it :) And when we're at it, I'd have to think about the namespace, not sure if \Google\ is the right one here.

@chregu
Copy link
Owner

chregu commented Mar 18, 2012

ONe more question out of curiosity:
"Update code to work with php5.3" sounds like it didn't work with PHP 5.3? Is that so? If yes, please tell me why, I'm eager to learn.
If not, you did the contrary "Update the code to not work with PHP 5.2 anymore" ;)

@michaelklishin
Copy link

@chregu compatibility with version X of PHP does not necessarily imply incompatibility with version Y. Maybe adding GoogleAuthenticator to travis-ci.org to test against multiple PHP versions is a good idea. Get started and see our PHP-centric guide if you want to learn more.

@rande
Copy link
Author

rande commented Mar 18, 2012

Well, PHP 5.2 is not supported anymore, imho it is time to move ;)

So yes, this PR broke the php5.2 compatibility, and also clean the code a bit :

  • Convention Standard
  • Unit Test
  • Travis CI

@chregu
Copy link
Owner

chregu commented Mar 18, 2012

@michaelklishin The code ran as well with PHP 5.3 (and 5.4) and I know travis-ci :) I'm just a little baffled usually to add namespaces just for the sake of it.

@rande I will move the library to the Liip account, name it properly and add namespaces. For that I would still be very happy, if you could separate your changes in different commits and separate the things you did.

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.

3 participants