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

Avoid calling getFingerprint where not necesarry #173

Merged
merged 1 commit into from
Feb 28, 2017
Merged

Conversation

thiemowmde
Copy link
Contributor

This goes along with wmde/WikibaseDataModel#717.

@thiemowmde thiemowmde modified the milestones: 3.7.0, 3.8.0 Feb 27, 2017
@@ -37,7 +36,7 @@ public function patchAliasGroupList( AliasGroupList $groups, Diff $patch ) {
}

/**
* @see MapPatcher
* @see Diff\Patcher\MapPatcher
Copy link
Member

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My PHPStorm does not understand this if it is not a full class name.

Copy link
Member

Choose a reason for hiding this comment

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

oh, I didn't notice there is no use Diff\Patcher\MapPatcher on top of the file (which makes the tool not able to understand the @see thing, which makes sense).

Copy link
Member

@manicki manicki Feb 28, 2017

Choose a reason for hiding this comment

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

meh, I am still half-awake. Why removing the import here then? I don't know all stuff PHP does internally but having the "use" statement doesn't mean the class is loaded, it only gets loaded once used in code (ie. not in the documentation). So this import wouldn't make any change regarding performance, memory usage etc as far as I get it (but maybe I am wrong).

I am not opposing to this change here. I am just a bit surprised as I've seen multiple places in Wikibase which import things just to have a simpler reference in the phpdoc to the class/its method, so I thought this is a general practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stuff in the use section is nothing but a string map and not loading anything. I'm aware of that.

What you have seen are @return, @param and @var tags. This here is a @see. PHPStorm does not consider the use section when resolving @see tags (why should it?). @see only works with local methods, classes that are in the same namespace, or fully qualified class names.

Copy link
Member

@manicki manicki Feb 28, 2017

Choose a reason for hiding this comment

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

I am confused by this evil tool. So my Phpstorm recognizes MapPatcher in this @see link when not using FQN. I thought it only happens with "use" import but apparently it is smart enough even when "use" is not there.
It seems like a tool-specific thing, I don't care then.

Per this part of your comment:

What you have seen are @return, @param and @var tags. This here is a @see.

So what you're saying is there is a rule that imported classes are considered while resolving @return, @param and @var comment but not in case of a @see? Fine if there is something like this somewhere but it seems like a good reason to be confused about :)

Copy link
Contributor Author

@thiemowmde thiemowmde Feb 28, 2017

Choose a reason for hiding this comment

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

@return, @param and @var describe actual behaviour of the code, while @see is more like an <a href> to guide the reader. Considering this the difference makes sense to me.

@manicki
Copy link
Member

manicki commented Feb 28, 2017

+1
Looks good, I am going to merge this.
Just giving @thiemowmde few minutes to read my ridiculous comment before I hit the button.

@manicki
Copy link
Member

manicki commented Feb 28, 2017

PR is fine. @manicki can take his fascinating monologue on being confused by IDE behaviour to some other place.

@manicki manicki merged commit 28508dc into master Feb 28, 2017
@manicki manicki deleted the unFingerprint branch February 28, 2017 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants