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

Merge IP constructors into base class #21

Closed
wants to merge 6 commits into from

Conversation

samuelwilliams
Copy link
Contributor

No description provided.

@samuelwilliams
Copy link
Contributor Author

Rather than go through a rebase - it was simpler to copy paste the code over into a new branch.

@samuelwilliams
Copy link
Contributor Author

@rlanvin - What are your thoughts on the PRs?

src/IP.php Outdated
/**
* @param int $ip
*/
private function fromInt($ip)
Copy link
Owner

Choose a reason for hiding this comment

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

For the from* methods, it would be nice to have type hinting. As discussed in #8 it makes sense to only support PHP >= 7.

I also think these from* should be static and return $ip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say "should return $ip", do you mean return the \GMP instance or an instance of PhpIP\IP?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean the GMP instance, so in the constructor it would look like "$this->ip = self::fromInt($ip)".

@rlanvin
Copy link
Owner

rlanvin commented Feb 23, 2019

@samuelwilliams My apologies. I got carried away by other projects and completely forgot about the open PRs here. Thanks for reminding me.

@samuelwilliams
Copy link
Contributor Author

Do you want this PR to change the minimum PHP requirements in composer.json and .travis.yml in order to move to PHP 7?

@rlanvin
Copy link
Owner

rlanvin commented Feb 23, 2019

@samuelwilliams No please make a separate independent PR for this. And it should not have any dependencies or include any other commits from another other currently open PRs.

@samuelwilliams samuelwilliams deleted the IP.__construct branch February 24, 2019 00:58
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.

2 participants