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

PhpStorm inspections #41

Closed
wants to merge 4 commits into from
Closed

PhpStorm inspections #41

wants to merge 4 commits into from

Conversation

SOF3
Copy link
Member

@SOF3 SOF3 commented Oct 21, 2016

Fixed some minor bugs and dropped some obsolete code

pocketmine\level\generator namespace is ignored in this commit

Also recommended to merge into 0.16 branch

Fixed some minor bugs and dropped some obsolete code

pocketmine\level\generator namespace is ignored in this commit
@SOF3 SOF3 added Type: Fix Bug fix, typo fix, or any other fix Status: Insufficiently Tested Category: Cosmetic Related to formatting or code style Category: Other labels Oct 21, 2016
@@ -1528,6 +1528,7 @@ public function close(){
* @param int $type
* @param mixed $value
*
* @param bool $send
Copy link
Member

Choose a reason for hiding this comment

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

this seems misplaced

@@ -453,7 +453,7 @@ public function sendSlot($index, $target){
* @return Human|Player
*/
public function getHolder(){
return parent::getHolder();
return $this->holder;
Copy link
Member

Choose a reason for hiding this comment

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

This override only seems to be here for the documentation. Pretty much pointless.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are already other instances of these overrides. They should not affect performance, and they are indeed necessary for IDEs to identify the type (for autocompletion) in a loosely typed language like PHP.

My change only carries out premature optimization to the method, and does not really change anything that can be worse.

use pocketmine\nbt\tag\ShortTag;
use pocketmine\nbt\tag\StringTag;
use pocketmine\nbt\tag\Tag;
use pocketmine\utils\Binary;
Copy link
Member

Choose a reason for hiding this comment

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

This, again!

Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

This will break the preprocessor again. Please make corrections.

@@ -589,6 +590,10 @@ public function clearCustomName(){
return $this;
}

/**
* @param $name
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably needs to be changed to @param string $name

@@ -28,7 +28,7 @@
use pocketmine\utils\PluginException;

abstract class MetadataStore{
/** @var \WeakMap[] */
Copy link
Member Author

Choose a reason for hiding this comment

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

Might want to comment this line out instead.

@@ -180,7 +180,7 @@ public function shutdown(){
protected function send($message, $level, $prefix, $color){
$now = time();

$thread = \Thread::getCurrentThread();
$thread = Thread::getCurrentThread();
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is for better IDE autocompletion. You might want to revert this change.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Eh?

@SOF3 SOF3 closed this Nov 6, 2016
@SOF3 SOF3 added the Resolution: Obsolete Superseded by other changes label Nov 6, 2016
@SOF3
Copy link
Member Author

SOF3 commented Nov 6, 2016

Obsoleted by #87

@SOF3 SOF3 deleted the isp branch November 6, 2016 03:50
@pmmp pmmp locked and limited conversation to collaborators Nov 6, 2016
@dktapps dktapps added Category: Core Related to internal functionality Multiple changes PR contains multiple changes and requires separation Category: Documentation Related to any documentation for PocketMine-MP and removed Status: Insufficiently Tested Category: Cosmetic Related to formatting or code style Category: Other labels Nov 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: Core Related to internal functionality Category: Documentation Related to any documentation for PocketMine-MP Multiple changes PR contains multiple changes and requires separation Resolution: Obsolete Superseded by other changes Type: Fix Bug fix, typo fix, or any other fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants