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

Javascript strings completion does not include the initial quote #671

Closed
arthurflachs opened this issue Dec 21, 2016 · 8 comments · Fixed by ycm-core/YouCompleteMe#2489
Closed

Comments

@arthurflachs
Copy link

arthurflachs commented Dec 21, 2016

Autocompleting javascript string (including required modules) with YCM does not happen as expected in Vim. The same behavior should be met with other clients.

Steps to reproduce

  • Enter the following : require('re
  • Select 'react'
    Expected result: require('react'
    Actual result: require(''react'

Solution to the problem
The ternjs server includes the quote in the completions result. Therefore the first call to the omnifunc function should return the index of the quote, not the index of the fist alphabetic character.
The part to change to make this working is to include the quote in the javascript FILETYPE_TO_IDENTIFIER_REGEX here : identifiers_utils.py

This solves the problem in this specific case in Vim, but I don't know if this might have unexpected side effects.

@puremourning
Copy link
Member

Could you post your .tern-project file ?

I can't get tern to complete the paths in require in my testing.

@arthurflachs
Copy link
Author

The node plugin needs to be enabled

{
  "plugins": {
    "node": {}
  }
}

@puremourning
Copy link
Member

OK thanks, I can reproduce.

I suspect we should actually strip the ' because i don't think ' is actually valid in a javascript identifier (i.e. you can't have a variable named ')

@puremourning
Copy link
Member

puremourning commented Jan 3, 2017

Steps to reproduce:

  • mkdir ~/tmp/js
  • create ~/tmp/js/.tern-project with the following contents:
{
  "plugins": {
    "node": {}
  }
}
  • save/quit
  • cd ~/tmp/js
  • vim test.js
  • insert require( 're and press <ctrl+space>
  • select item 'repl'
  • expect: require( 'repl'
  • actual require( ''repl'

@bstaletic
Copy link
Collaborator

I'm not a javascript developer, so I am not quite sure my patch is sufficient, but it does solve the ssue described in the previous @puremourning's post.

Here's what I came up with:

diff --git a/ycmd/identifier_utils.py b/ycmd/identifier_utils.py
index 6f51fe6b..e64cf4a5 100644
--- a/ycmd/identifier_utils.py
+++ b/ycmd/identifier_utils.py
@@ -68,7 +68,7 @@ FILETYPE_TO_IDENTIFIER_REGEX = {
     # Spec:
     # http://www.ecma-international.org/ecma-262/6.0/#sec-names-and-keywords
     # Default identifier plus the dollar sign.
-    'javascript': re.compile( r"([^\W\d]|\$)[\w$]*", re.UNICODE ),
+    'javascript': re.compile( r"([^\W\d]|\$|')[\w$]*", re.UNICODE ),

     # Spec: http://www.w3.org/TR/CSS2/syndata.html#characters
     # Good summary: http://stackoverflow.com/a/449000/1672783

Is that all that is needed to fix the issue and not create a different problem somewhere else? If so I could submit a PR.

@puremourning
Copy link
Member

Like i said above

i don't think ' is actually valid in a javascript identifier (i.e. you can't have a variable named ')

@bstaletic
Copy link
Collaborator

Would changing r"([^\W\d]|\$)[\w$]*" to r"(([^\W\d]|\$)[\w$]*|'([^\W\d]|\$)[\w$]*')" work?

This is essentially changing "default plus the dollar sign" to "default plus the dollar sign or the same, but surrounded with single quotes".

If not, would changing the order of the "or" operands make a difference?

@puremourning
Copy link
Member

Changing the identifier regex is not the solution. It might work around this issue but causes many more serious ones.

zzbot added a commit that referenced this issue May 20, 2017
[RFC] Allow completers to set the start column

We always calculate the start column based on our own identifier semantics.

However, some completion engines might have a different interpretation, and indeed the javascript completer does exactly that for `require( '`

Fixes: #671

This PR is just to demonstrate the approach for comments. It isn't really tested yet (the tests will certainly fail).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/681)
<!-- Reviewable:end -->
zzbot added a commit that referenced this issue Jun 30, 2017
[READY] Allow completers to set the start column

We always calculate the start column based on our own identifier semantics.

However, some completion engines might have a different interpretation, and indeed the javascript completer does exactly that for `require( '`

Fixes: #671

This PR is just to demonstrate the approach for comments. It isn't really tested yet (the tests will certainly fail).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/681)
<!-- Reviewable:end -->
zzbot added a commit that referenced this issue Jun 30, 2017
[READY] Allow completers to set the start column

We always calculate the start column based on our own identifier semantics.

However, some completion engines might have a different interpretation, and indeed the javascript completer does exactly that for `require( '`

Fixes: #671

This PR is just to demonstrate the approach for comments. It isn't really tested yet (the tests will certainly fail).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/681)
<!-- Reviewable:end -->
zzbot added a commit to ycm-core/YouCompleteMe that referenced this issue Jul 3, 2017
[READY] Reset the start column in omnifunc completer

# PR Prelude

Thank you for working on YCM! :)

**Please complete these steps and check these boxes (by putting an `x` inside
the brackets) _before_ filing your PR:**

- [X] I have read and understood YCM's [CONTRIBUTING][cont] document.
- [X] I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
- [X] I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
- [X] **I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.**

# Why this change is necessary and useful

A number of issues have been raised over the years where YCM doesn't interact great with the user's omnifunc. This is commonly because we ignore the omnifunc's `findstart` response, and use our own implementation (`base.CompletionStartColumn`).

In combination with ycm-core/ycmd#681 this change uses the omnifunc's start column for completions and fixes ycm-core/ycmd#671 and others, such as:

- #1322 probably (not yet tested)
- #1957
- #1816

Note: This is just an initial test for sharing the code to gauge reaction. Not fully tested yet.

[cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md
[code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2489)
<!-- Reviewable:end -->
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 a pull request may close this issue.

3 participants