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

Support type hinting #352

Closed
wants to merge 7 commits into from
Closed

Support type hinting #352

wants to merge 7 commits into from

Conversation

jens1o
Copy link
Contributor

@jens1o jens1o commented Apr 12, 2017

This pr does include the pr #374, because the pr was created with the master branch. So I would like that the "define-constants" pr will be merged first.

@jens1o jens1o requested a review from felixfbecker April 12, 2017 08:08
@jens1o
Copy link
Contributor Author

jens1o commented Apr 12, 2017

When fine, I'll create tests. But I'm unsure wether the code is okay like this.

@codecov
Copy link

codecov bot commented Apr 12, 2017

Codecov Report

Merging #352 into master will decrease coverage by 0.82%.
The diff coverage is 61.76%.

@@             Coverage Diff              @@
##             master     #352      +/-   ##
============================================
- Coverage      84.2%   83.38%   -0.83%     
- Complexity      815      834      +19     
============================================
  Files            59       59              
  Lines          1722     1751      +29     
============================================
+ Hits           1450     1460      +10     
- Misses          272      291      +19
Impacted Files Coverage Δ Complexity Δ
src/Index/Index.php 68.62% <ø> (ø) 21 <0> (ø) ⬇️
src/Protocol/SymbolInformation.php 95.91% <100%> (+0.46%) 30 <0> (+5) ⬆️
src/DefinitionResolver.php 78.85% <50%> (-3.01%) 290 <0> (+14)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de6aed6...1b9d870. Read the comment docs.

@swiffer
Copy link

swiffer commented Sep 17, 2017

@felixfbecker is this still working, would be great to have this merged?

should solve issue #350 right?

@felixfbecker
Copy link
Owner

This is based on an old version of the langserver that used a different parser, see the merge conflicts. It's also missing some tests.

@jens1o
Copy link
Contributor Author

jens1o commented Sep 17, 2017

Okay, ehm, I will watch out when I got more time and then refile the pull request.

@jens1o jens1o closed this Sep 17, 2017
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