Source stubber based on jetbrains/phpstorm-stubs
#373
Conversation
kukulich
force-pushed
the
source-stubber
branch
from
76dcab5 to
5f677bb
Compare
|
@Ocramius just for your info: a lot of tests are missing here currently, I will write them later when I know you are ok with the interface etc :) |
kukulich
force-pushed
the
source-stubber
branch
2 times, most recently
from
c99097d to
0e54523
Compare
|
The repo is now tagged https://github.com/JetBrains/phpstorm-stubs/releases/tag/2018.1.2 :) |
|
that's one hell of a high |
kukulich
force-pushed
the
source-stubber
branch
3 times, most recently
from
93096e7 to
9149d75
Compare
|
@asgrim If there are no objections about the way how I've implemented it I can rebase it, add tests and prepare for final review :) |
|
@kukulich no objections here; seems fine to me 👍 |
|
Hello! I just started working on a similar effort (https://github.com/jcsilkey/BetterReflection/tree/stubber) for my own personal usage before I looked and saw this PR existed. I wasn't aiming to replace the internal stubs with the JetBrains stubs but rather a different set and therefore needed to refactor the SourceLocator to be able to pass in a custom Stubber instance. Anyhow, I see that this PR hasn't been updated in a quite a while and I'd be willing to consolidate the two and update it if there are no objections (particularly from @kukulich since this is his work). Thanks! |
|
@jcsilkey interesting: what is the use case you're trying to solve? BR can already load classes from files (e.g. General side note for @Ocramius / @kukulich - I wonder if it makes sense to replace the mechanism of |
|
I'd add a new source locator for now: we don't yet know the quality of the
phpstorm one, so we'll have to leave the old one in place for a while
…On Tue, 26 Feb 2019, 09:10 James Titcumb, ***@***.***> wrote:
@jcsilkey <https://github.com/jcsilkey> interesting: what is the use case
you're trying to solve? BR can already load classes from files (e.g.
SingleFileSourceLocator for single files, DirectoriesSourceLocator for
anything in a folder), so I'm wondering if they can't just be used. I'm
assuming you have a directory of PHP files for the stubs, otherwise, in
what form would you be supplying the stubs? Is it just because the JB stubs
aren't sufficient?
General side note for @Ocramius <https://github.com/Ocramius> / @kukulich
<https://github.com/kukulich> - I wonder if it makes sense to replace the
mechanism of PhpInternalSourceLocator with just a wrapper around
DirectoriesSourceLocator that points to the JB stubs...?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#373 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakAawq0AfyQdFLP3O6Wc8V2AhmoyHks5vROvcgaJpZM4Pg1QH>
.
|
|
I plan to rebase this PR in March. |
|
@kukulich Can you foresee when you'll be able to rebase this PR? Thanks! I am asking because of sebastianbergmann/phpunit#3577 (comment). |
|
@sebastianbergmann It’s in progress. Both chilldren were ill in March so less free time than expected :( I hope I will finish it in 14 days |
|
@kukulich Thanks for the update. Familiy first! Hope your children are no longer ill. |
kukulich
force-pushed
the
source-stubber
branch
from
9149d75 to
6b36778
Compare
|
The PR is rebased and build is green (Travis is green, AppVeyor is 💩 ) I have to still write new tests. |
|
@kukulich btw, sorry for not noticing this patch before! |
kukulich
force-pushed
the
source-stubber
branch
from
6b36778 to
d392fb7
Compare
kukulich
force-pushed
the
source-stubber
branch
from
d392fb7 to
de580a8
Compare
|
AppVeyor failures are coming from "standard Windows PHP environment weirdness", so they can be ignored. |
Ocramius
changed the title
jetbrains/phpstorm-stubs
|
Thanks @kukulich! |
| /** | ||
| * Determine if a stub exists for specified class name | ||
| */ | ||
| public function hasStub(string $className) : bool |
There was a problem hiding this comment.
For future readers: this is indeed a BC break, but since this is an internal component (which is erroneously not marked as @internal), we can ignore it.
|
@Ocramius You are really fast :) As I mentioned in previous comment, tests are missing. I’ll send PR with them ASAP (next week) |
|
Urghh, I'll need a coverage threshold plugin to be added to the build to prevent this. I expect to finish up another patch today, so we'll likely also have 3.4.0. |
| { | ||
| parent::__construct($astLocator); | ||
|
|
||
| $this->stubber = new SourceStubber(); | ||
| $this->stubber = $stubber ?? new ReflectionSourceStubber(); |
There was a problem hiding this comment.
shouldn't this include the phpstorm stubber as well?
There was a problem hiding this comment.
@theofidry No, because it would be BC break.
Proof of concept to have something to discuss.