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

Fix IndexError in Typescript completer when querying a subcommand without argument #263

Closed
wants to merge 1 commit into from

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Nov 27, 2015

When typing the command :YcmCompleter without argument in Vim, YCM will return the following error IndexError: list index out of range. This is because the Typescript completer, unlike other completers, does not check if arguments in OnUserCommand is defined.

Review on Reviewable

@puremourning
Copy link
Member

Hmm no reviewable.io?

@micbou
Copy link
Collaborator Author

micbou commented Nov 27, 2015

I messed up by editing my comment. This is fixed now.

@vheon
Copy link
Contributor

vheon commented Nov 27, 2015

LGTM


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from the review on Reviewable.io

Typescript completer did not handle the case where no argument was
specified when querying a command. Raises the appropriate exception
like other completers.
@puremourning
Copy link
Member

The professional in me feels that we should add a test, but LGTM if you don't feel it is worth it :)


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@puremourning
Copy link
Member

Reviewed 1 of 1 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@micbou
Copy link
Collaborator Author

micbou commented Nov 27, 2015

This is certainly doable. Let me take a shot at it.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Nov 27, 2015

Is doable and it would certainly make sense... but then what about all the other completers?


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@micbou
Copy link
Collaborator Author

micbou commented Nov 27, 2015

I am currently adding tests for all completers. I am wondering if there is a way to get all the supported filetypes by ycmd.
It would be very useful for this kind of tests.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Nov 27, 2015

Wouldn't be better to abstract this into the Completer class?


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Valloric
Copy link
Member

Yes, MOAR TESTS. @micbou I see you've said you're writing further tests. Have I mentioned how I think that's an absolutely brilliant idea? MOAR TESTS FOR TESTORUS, TEH ALMIGHTY TEST GOD. HE WELCOMES YOUR NON-FLAKY OFFERINGS.

@vheon Pulling this out into Completer might be a good idea. I can't imagine how a raw :YcmCompleter call without even a subcommand name could be useful.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@micbou
Copy link
Collaborator Author

micbou commented Nov 28, 2015

Pulling this out into Completer might be a good idea. I can't imagine how a raw :YcmCompleter call without even a subcommand name could be useful.

This is the new direction I am taking but some changes in the completer API are needed. My current approach is to add a new abstract method GetSubcommandsMap in the Completer class returning a map between the subcommands and the Completer methods to call. This map is then used in DefinedSubcommands and OnUserCommand methods so that Completers don't need to implement them, only the map.

I'll close this PR and send a new one implementing this if you think this is the right approach.


Comments from the review on Reviewable.io

@vheon
Copy link
Contributor

vheon commented Nov 29, 2015

This is roughly the direction that I had in mind, so for me go ahead to the new PR.


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@micbou
Copy link
Collaborator Author

micbou commented Nov 29, 2015

Closing this in favor of PR #264.


Comments from the review on Reviewable.io

@micbou micbou closed this Nov 29, 2015
homu added a commit that referenced this pull request Nov 29, 2015
Move subcommands logic to Completer class

Currently, each semantic completer reimplements the `OnUserCommand` and `DefinedSubcommands` in about the same way. To avoid duplication of code, this PR moves the logic of these methods to the `Completer` class. Completers only need to implement a new function `GetSubcommandsMap` that return a map between the subcommands name and the methods to call. The map structure is explained in the method documentation.

Minor improvements:
 - `DefinedSubcommands` now returns an alphabetically sorted list (tests updated to reflect this);
 - Fix issue in PR #263.

Important: if this PR is merged, the `OmniCompleter` class will need to be updated in YCM by inheriting from `GeneralCompleter` instead of `Completer` or implementing the `GetSubcommandsMap` method.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/264)
<!-- Reviewable:end -->
@micbou micbou deleted the typescript-completer branch November 29, 2015 20:37
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.

4 participants