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

add PHPStan and fix found issues #157

Closed
wants to merge 6 commits into from
Closed

Conversation

JanTvrdik
Copy link
Contributor

No description provided.

@f3l1x
Copy link
Member

f3l1x commented Dec 2, 2017

Good job. One thing is troubling me, it means that only nette/utils has defined propel phpDoc and other packages have not. Do you plan at it to the others, too?

@JanTvrdik
Copy link
Contributor Author

I don't see how that is relevant. It it forbidden to fix bug in one package without fixing bugs in all packages?

@f3l1x
Copy link
Member

f3l1x commented Dec 2, 2017

Nope, don't get me wrong I like the change.

	/**
	 * Replaces or appends a item.
	 * @param  int|null
	 * @return void
	 * @throws Nette\OutOfRangeException
	 */
	public function offsetSet($index, $value)

=>

	/**
	 * Replaces or appends a item.
	 * @param  int|null $index
	 * @return void
	 * @throws Nette\OutOfRangeException
	 */
	public function offsetSet($index, $value)

But, we should do it for all packages. To keep it same.

I don't want to add phpdoc to one method and not to others, right?

@dg
Copy link
Member

dg commented Feb 6, 2018

Great work. I merged most of issues 32fd477

I have not added PhpStan yet, I must first become familiar with it.

Also I did not add the parameter $names in phpDoc because it is not used in another places.

@JanTvrdik
Copy link
Contributor Author

Also I did not add the parameter $names in phpDoc because it is not used in another places.

What places? You mean other nette/* packages?

@dg
Copy link
Member

dg commented Feb 6, 2018

Yes.

@JanTvrdik
Copy link
Contributor Author

@dg Well, we can't add this to all packages at once. One package will always need to be the first. Also we've already had a related discussion few years ago.

@dg
Copy link
Member

dg commented Feb 9, 2018

Yes, RFC was accepted.

@dg dg force-pushed the master branch 3 times, most recently from 8aa61b9 to fd48510 Compare February 19, 2018 14:44
@dg dg closed this in f0af5a0 Apr 2, 2018
dg pushed a commit that referenced this pull request Apr 2, 2018
@dg
Copy link
Member

dg commented Apr 2, 2018

@JanTvrdik how can be solved this error? https://travis-ci.org/nette/utils/jobs/361331623

@Majkl578
Copy link
Contributor

Majkl578 commented Apr 2, 2018

Needs to be added to ignored errors: https://github.com/nette/utils/blob/master/phpstan.neon#L2

@JanTvrdik
Copy link
Contributor Author

@dg Here are your options:

  • fix PHP configuration on Travis, so that the imagewebp() function actually exists
  • provide dummy (stub) definition of imagewebp() in PHPStan bootstrap file
  • ignore the error

@dg
Copy link
Member

dg commented Apr 2, 2018

How can I fix PHP configuration on Travis?

@Majkl578
Copy link
Contributor

Majkl578 commented Apr 2, 2018

I don't think you can fix it, that's why I didn't suggest it. PHP must be compiled with WEBP support, but it obviously isn't on Travis. :/
http://php.net/manual/en/image.installation.php

dg pushed a commit that referenced this pull request Apr 3, 2018
dg pushed a commit that referenced this pull request Apr 3, 2018
dg pushed a commit that referenced this pull request Apr 3, 2018
dg pushed a commit that referenced this pull request Apr 4, 2018
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.

None yet

4 participants