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

experiment with psalm detection of unused code #2263

Closed
wants to merge 6 commits into from
Closed

experiment with psalm detection of unused code #2263

wants to merge 6 commits into from

Conversation

orklah
Copy link
Contributor

@orklah orklah commented Feb 4, 2020

I ran Psalm with it's unused code detection on src/

I will run the CI on this and restore all broken functionnality and see what's left.

I know some code is probably needed for BC purposes but it will at least highlight untested code if there is any...

(This is actually the first time I use the option in psalm, I also want to see what it can do :) )

@orklah
Copy link
Contributor Author

orklah commented Feb 4, 2020

I was starting to worry I'd have to revert everything :)

There's still some interesting catches though.

Mind to take a look at it? Is there anything interesting to delete?

@jaapio
Copy link
Member

jaapio commented Feb 5, 2020

I'm not sure if we can use this output. Because we do not test everything in phpDocumentor, so I'm not sure if it would break anything :-)

@orklah
Copy link
Contributor Author

orklah commented Feb 5, 2020

@jaapio you may still want to look at https://github.com/phpDocumentor/phpDocumentor/pull/2263/files#diff-5844ff81b5eb5ff658aabe38d06279fdL70
I'm not sure what the intent was but it seems weird. We cast a Dsn to string a put the result in a variable while being in a try block.

Said variable is not used later so it could be dropped. It leaves only the cast to string in the try catch.

The __toString method of Dsn don't declare throwing Exception and it doesn't seem like it does. Even if it did, throwing exception in __toString is only possible on PHP 7.4 and PhpDocumentor must be compatible with ^7.2.5

I posted this on psalm issues because while this code could be valid, I don't think it is in this case.

@jaapio
Copy link
Member

jaapio commented Feb 5, 2020

I think you are right, this catch block should be removed. You can do that in a separate PR. Other things are not removable I think.

@orklah orklah mentioned this pull request Feb 5, 2020
@orklah
Copy link
Contributor Author

orklah commented Feb 5, 2020

PR created, I close this one

@orklah orklah closed this Feb 5, 2020
@orklah orklah deleted the psalm-unused branch February 22, 2020 12:43
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

2 participants