Skip to content

Conversation

@gatesn
Copy link
Contributor

@gatesn gatesn commented Jan 10, 2018

@lgeiger does this work for you?

Fixes #212
Fixes #189

lgeiger and others added 4 commits January 10, 2018 11:21
This allows the user to choose between Rope and Jedi for completions.

It also remove the pluggy racing since Jedi's cache isn't thread safe.

Closes #189
@lgeiger
Copy link
Contributor

lgeiger commented Jan 10, 2018

With this the user is allowed to enable both completion providers and would receive duplicated completions.

I think this would be a nicer configuration interface for users. What do you think.

I can test this branch tomorrow and provide detailed feedback.

@gatesn
Copy link
Contributor Author

gatesn commented Jan 10, 2018

So we could either pass @hookspec(firstresult=True) to the completions hook, meaning we'll use the first non-None result. Or we merge and try to filter duplicates.

I'd rather not have config to switch between them because it means the user needs to understand the pros/cons of each. Also, it's similar behaviour to firstresult=True, but less powerful since there's no fallback.

The disadvantage of first result (and your solution too), is that it becomes impossible for a plugin to add extra completions into the mix. Which was the point of plugins in the first place.

@lgeiger
Copy link
Contributor

lgeiger commented Jan 12, 2018

I think this is the most flexible solution for now. We can think about merging to filter duplicates in the future once people start using this.

As for the configuration, editor integrations can always decide for them selves to present a different config schema to the users.

@gatesn gatesn merged commit af4dcb3 into develop Jan 14, 2018
@gatesn gatesn deleted the ngates/remove-pluggy-racing branch January 14, 2018 11:39
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