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 phpDoc to use Fully Qualified Class Name #720

Closed
wants to merge 1 commit into from
Closed

Update phpDoc to use Fully Qualified Class Name #720

wants to merge 1 commit into from

Conversation

@barryvdh
Copy link
Contributor

@barryvdh barryvdh commented Mar 27, 2013

PR for Issue #701 Changes phpDoc namespace to FQN, following the
official phpdoc documentation:
http://phpdoc.org/docs/latest/for-users/types#valid-class-name

PR for Issue #701 Changes phpDoc namespace to FQN, following the
official phpdoc documentation:
http://phpdoc.org/docs/latest/for-users/types#valid-class-name
@RSully
Copy link
Contributor

@RSully RSully commented Mar 27, 2013

I can't tell from the web interface what is going on (my first guess is tabs vs spaces) but for files like src/Illuminate/Auth/AuthManager.php any idea why the diff is a complete remove/add of the file?

Also if we're going to do all of these changes might it be wise to also consider either removing @return from __constructor or switching to @return self as per phpdoc.org (see #702)

@barryvdh
Copy link
Contributor Author

@barryvdh barryvdh commented Mar 27, 2013

Hmm, I'm not sure. All I did was replace all @return Illuminate\Class\Name to @return \Illuminate\Class\Name (and @param and @var), so it is fully qualified, instead of in the local namespace (otherwise, a reference to Illuminate\Database\Query\Expression from within Illuminate\Database will refer to Illuminate\Database\Illuminate\Database\Query\Expression.
Don't know why Github has to replace more lines than this.

}

}
<?php namespace Illuminate\Workbench;

This comment has been minimized.

@bencorlett

bencorlett Mar 27, 2013
Contributor

What's with half of these files files having EVERY line changed?!


}

}

This comment has been minimized.

@bencorlett

bencorlett Mar 27, 2013
Contributor

And here

This comment has been minimized.

@barryvdh

barryvdh Mar 27, 2013
Author Contributor

Yeah sorry, I just did a couple of find and replaces in notepad++ and
committet them. Don't know why github thinks I changed all those lines.
Op 27 mrt. 2013 22:05 schreef "Ben Corlett" notifications@github.com het
volgende:

In src/Illuminate/View/ViewServiceProvider.php:

  • * @param Illuminate\Foundation\Application $app
  • * @return bool
  • */
  • public function sessionHasErrors($app)
  • {

- $config = $app['config']['session'];

  •   if (isset($app['session']) and ! is_null($config['driver']))
    
  •   {
    
  •       return $app['session']->has('errors');
    

- }

- }

-}

And here


Reply to this email directly or view it on GitHubhttps://github.com//pull/720/files#r3560166
.

This comment has been minimized.

@bencorlett

bencorlett Mar 27, 2013
Contributor

GitHub isn't thinking you've changed all those files, git is. Which means you have :P.

Try a decent editor like Sublime Text 2 :) But see my other comment.

This comment has been minimized.

@Anahkiasen

Anahkiasen Mar 27, 2013
Contributor

I'm thinking indentation issues.

@bencorlett
Copy link
Contributor

@bencorlett bencorlett commented Mar 27, 2013

You really need to re-do this PR to be honest. That issue with every line being changed in half of the files is no good.

However, I'm not sure if your'e wasting your time or not. There was an issue open which Taylor has closed (which you've referred to in your PR, #701). I don't like your chances of having it merged.

The idea of proposals & issues before-hand is to see the likelihood of something getting merged, so you don't feel you've wasted all your time if it doesn't.

@barryvdh
Copy link
Contributor Author

@barryvdh barryvdh commented Mar 27, 2013

I know Taylor closed this, but the reason was that using local classnames
(Router instead of Illiminate/Routing/Router) didn't make it directly clear
what the real class is. Using fully qualified names doesn't have this
disadvantage..

And I did try to do it with Sublime Text just now, but got the same result. So I'm going to let Taylor decide what he want to do with it. It's an easy search&replace action, so it doesn't have to be this PR.

@bencorlett
Copy link
Contributor

@bencorlett bencorlett commented Mar 27, 2013

It may be something to do with encoding or your git configuration. Are you on a PC by any chance?
On 28/03/2013, at 8:45 AM, barryvdh notifications@github.com wrote:

I know Taylor closed this, but the reason was that using local classnames
(Router instead of Illiminate/Routing/Router) didn't make it directly clear
what the real class is. Using fully qualified names doesn't have this
disadvantage..

And I did try to do it with Sublime Text just now, but got the same result. So I'm going to let Taylor decide what he want to do with it. It's an easy search&replace action, so it doesn't have to be this PR.


Reply to this email directly or view it on GitHub.

@barryvdh
Copy link
Contributor Author

@barryvdh barryvdh commented Mar 27, 2013

Yes, Windows 8 with Github for Windows, with a fresh install of Sublime Text 2

@bencorlett
Copy link
Contributor

@bencorlett bencorlett commented Mar 27, 2013

Hmm. yeah I've seen people have issues with windows, line endings and encoding. I'm on a mac so I'm not much help.
On 28/03/2013, at 8:49 AM, barryvdh notifications@github.com wrote:

Yes, Windows 8 with Github for Windows, with a fresh install of Sublime Text 2


Reply to this email directly or view it on GitHub.

@jonphipps
Copy link

@jonphipps jonphipps commented Mar 27, 2013

I've found it necessary to tell git explicitly that .php files (among others) are text in order to get the diffs correct

#~/.gitattributes_global
*.csv text
*.ttl text
*.php text
*.inc text
*.md text
*.markdown text
*.xsl text

This page: https://help.github.com/articles/dealing-with-line-endings#platform-windows
should help with the line-endings problem. It doesn't look like an indent problem, since you're using tabs like Laravel.

This is a good fix for those few of us NOT using Sublime. :-) But it would probably be good to try to get a formal proposal accepted since it affects a lot of files.

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Mar 29, 2013

I'm not comfortable merging this with those entire file replacements being shown in the diff.

@barryvdh
Copy link
Contributor Author

@barryvdh barryvdh commented Mar 29, 2013

Could you just use Find in files and replace all @param Illuminate\ to @param \Illuminate, and the same form @var and @return? I found them al with 1-3 spaces.
Or do you want me to try a new PR?

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Mar 29, 2013

Done.

@barryvdh
Copy link
Contributor Author

@barryvdh barryvdh commented Mar 29, 2013

Thanks!

@barryvdh barryvdh deleted the barryvdh:patch/phpdoc-fqn branch Mar 29, 2013
@barryvdh
Copy link
Contributor Author

@barryvdh barryvdh commented Mar 29, 2013

To be totally correct, all the Symfony and other classes should also be prefixed with a , but perhaps that is for a future time ;)
(And there are some more things wrong with the phpdocs, like CookieJar and Filesystem references that do not exist)

@zantx3
Copy link

@zantx3 zantx3 commented Sep 26, 2016

submit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants