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

Use type hinting #16

Closed
wants to merge 1 commit into from
Closed

Use type hinting #16

wants to merge 1 commit into from

Conversation

kzbandai
Copy link

Hello, the purpose of this PR is that uses type hinting added on PHP7.
I thought that I want to add type hinting if composer.json restrict only using PHP7.
If you like this PR, could you merge into master ? Thank you.

@djekl
Copy link

djekl commented May 29, 2017

Considering Guzzle has a minimum PHP version of 5.5, shouldn't this repo also have the same? And thus not support PHP7's typehinting. All considering that this is a wrapper around Guzzle.

@kzbandai
Copy link
Author

@djekl I don't think so. Because in composer.json, that declares only using PHP7.
Could you read this line ?
https://github.com/kitetail/zttp/blob/master/composer.json#L13

@djekl
Copy link

djekl commented May 29, 2017

I agree with you there, and understand why you have done it. My question was aimed at why doesn't this package have a minimum version of 5.5 to support all developers using Guzzle, not just ones using PHP7

@kzbandai
Copy link
Author

I don't know the reason because I'm not owner or maintainer on this repository.
But I guess the reason that the security support of php5.5 has already finished and of php5.6 will finish at 12/31/2018.

we(meaning engineers all over the world) have to use PHP7 or higher more positively, I think.

@djekl
Copy link

djekl commented May 29, 2017

It wasn't aimed directly at yourself, more towards @adamwathan. Again I understand and agree with you, but if the underlying code that this extends supports PHP5.5+ then I believe so should this.

@djekl
Copy link

djekl commented May 29, 2017

If this were standalone code, and didn't extend anything, then I would say yes to PHP7+

@adamwathan
Copy link
Contributor

Thanks for your efforts @kzbandai!

Type-hints are left out intentionally, I don't use them in my code and have pretty strong opinions against them (can listen to this podcast episode if you're interested in more).

That aside, this library is also a bit of a fun exercise in doing things differently just to be contrarian, for example:

  • No type hints
  • No visibility keywords (public, private, etc.)
  • No property declaration (use constructor only instead)
  • Multiple classes in one file

...and some other fun self-imposed limitations on code-style like:

  • No conditionals
  • No temporary variables
  • Each method has to return immediately; no more than one logical line per function

Again, all just to have a bit of fun 😊 Life's too short to take everything so seriously.

I'm not interested in supporting anything below PHP 7 personally; and in fact this library is already using a PHP 7 feature by having a method called new, which won't work in earlier versions of PHP.

@adamwathan adamwathan closed this May 29, 2017
@kzbandai
Copy link
Author

kzbandai commented May 29, 2017

@adamwathan Thank you for your response and reaction.
I understood your thinking method and the thought of this repository deeply.
And I'm happy now that I learned from your the thought you described.

Life's too short to take everything so seriously.

I impressed with this word.

I don't regret creating this PR, and I am grateful to you. 😃

@kzbandai kzbandai deleted the use-type-hinting branch May 29, 2017 16:41
@adamwathan
Copy link
Contributor

You shouldn't regret it at all! It's always hard to close PRs, feel really guilty every time. Don't hesitate to contribute more stuff in the future, even if I don't want to accept a change I still greatly appreciate the effort put in to contribute it.

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.

None yet

5 participants