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

[5.7] Cookie::get() doc block fixed. #26334

Merged
merged 1 commit into from
Nov 1, 2018
Merged

[5.7] Cookie::get() doc block fixed. #26334

merged 1 commit into from
Nov 1, 2018

Conversation

dmitry-ivanov
Copy link
Contributor

Cookie::get() returns not only the string, but also the array and null, as the underlying Http\Request does.

$array = Cookie::get();
$null = Cookie::get('foo');

@taylorotwell taylorotwell merged commit 8699a62 into laravel:5.7 Nov 1, 2018
@dmitry-ivanov
Copy link
Contributor Author

dmitry-ivanov commented Nov 1, 2018

Hey, @taylorotwell , I've also noticed that the framework codebase is not always following its own Contribution Guide.

Guide says that:

Note that the @param attribute is followed by two spaces, the argument type, two more spaces, and finally the variable name.

https://laravel.com/docs/5.7/contributions#coding-style

But I've done the grep search and found that there are many places where we do have three, four, five ... and even ten spaces.

In some cases it looks fine, it used for indentation, for example like here (3 spaces before $parameters):

    /**
     * Dynamically call the default driver instance.
     *
     * @param  string  $method
     * @param  array   $parameters
     * @return mixed
     */

But in other cases it has no sense, for example like here (3 spaces before $abilities):

    /**
     * Define abilities for a resource.
     *
     * @param  string  $name
     * @param  string  $class
     * @param  array|null   $abilities
     * @return $this
     */

Probably three spaces were used for an indentation initially, but then the $abilities became nullable (|null added) and those three spaces became senseless.

So, what do you think about that?

I can do a PR with "two-spaces everywhere" or fix just those places where it has no sense (but in that case, probably, you should update the Contribution Guide).

@mfn
Copy link
Contributor

mfn commented Nov 1, 2018

I just wished the project would use "simpler" guidelines which don't need this attention to detail. It's so off-putting at times, really.

Off the top of my head:

  • this "2 spaces" thing
  • then the alignment of the parameters
    Just add a new one, longer, and you have tune the whole docblock. Nice diff, you get to see changes unrelated because formatting
  • Sorting imports by length
  • many more but I digress…

Or just activate StyleCI autocommit on PRs and save everyone time 😄

@dmitry-ivanov
Copy link
Contributor Author

@mfn Yeah, I agree with you. I think that one simple rule "2 spaces" is easier to remember and handle.

StyleCI is enabled for the Laravel Framework repo:
https://github.styleci.io/repos/7548986

But I think it doesn't have such check now. May be a new feature request for @GrahamCampbell

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.

3 participants