Skip to content

Conversation

@kermorgant
Copy link
Contributor

Sorry for last PR, it introduced a regression when completion occured on object methods...

This should hopefully fix that

@ejmr
Copy link

ejmr commented Jul 21, 2018

Thank you for catching the regression and correcting it. In my experience those things exist for way too long in too many open-source projects.

@kermorgant
Copy link
Contributor Author

I've been using thix fix for some days now and I haven't observed any problem.

@zonuexe I'm tempted to merge this one as I can only see benefits with it. would you be ok for a merge ?

@ejmr
Copy link

ejmr commented Jul 25, 2018

The commit message has a typo and IMO should begin with "Fix..." instead of "fix...". That small comment aside though, I reviewed the code (it's a small change), and I will vouch for its quality and that it should be merged into PHP Mode.

@kermorgant
Copy link
Contributor Author

kermorgant commented Jul 25, 2018

As a side note, I've noticed a pull request which might be worth following in regard to how completion works with company-phpactor.

I've not checked personnally how it would change things, but I thought adding this comment might be useful in the future.

@kermorgant kermorgant changed the title fix regression from pr #29 causing completion on object to fail Fix regression from pr #29 causing completion on object to fail Jul 25, 2018
@kermorgant
Copy link
Contributor Author

@ejmr are you suggesting to open a pull request directly for php-mode ?

@ejmr
Copy link

ejmr commented Jul 25, 2018

@kermorgant

...are you suggesting to open a pull request directly for php-mode?

No. I accidentally typed the wrong project name. Hilariously in a comment where I called you out on a typo, lol. I meant this change should be merged in phpactor.el.

pr#29 added avriable completion but completion of object methods
stopped working. This should now be fixed.
@kermorgant kermorgant force-pushed the var-completion-fix branch from 9e4df19 to 9f208d3 Compare July 26, 2018 06:55
@zonuexe
Copy link
Member

zonuexe commented Jul 26, 2018

@kermorgant LGTM

@zonuexe I'm tempted to merge this one as I can only see benefits with it. would you be ok for a merge?

You can merge such trivial bugs at your discretion. 👍

@zonuexe zonuexe merged commit 3696dfc into emacs-php:master Jul 26, 2018
@zonuexe
Copy link
Member

zonuexe commented Jul 26, 2018

BTW, I had a serious problem in managing the issue / PR's reading and resolution, but I installed Jasper app today. I am trying to improve my response by that.

@kermorgant kermorgant deleted the var-completion-fix branch September 16, 2018 11:55
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