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

Improving dict autocomplete #13735

Closed
mlucool opened this issue Aug 22, 2022 · 4 comments · Fixed by #13745
Closed

Improving dict autocomplete #13735

mlucool opened this issue Aug 22, 2022 · 4 comments · Fixed by #13745

Comments

@mlucool
Copy link

mlucool commented Aug 22, 2022

We have found the dict autocomplete to show too many option at times. As an example, given this:

$ ls
a-dir-name  a-file
$ ipython

The following is appears:
image

Here you can see a number of things are suggested that are not helpful including:

  1. Files
  2. Magics

This is made much worse by something like pyflyby where all possible imports that start with a are also listed.

Ideas:

  1. Only show valid keys of the dict (when possible)
  2. Sort to show dict keys first and all other options after
@krassowski
Copy link
Member

krassowski commented Aug 22, 2022

This is similar to a previous request to only show magics after % (#12959) which was solved by a simple check for % prefix in #13483. A similar issue about magics showing up in import completions was raised in #12987.

I was recently thinking about adding more capabilities to matchers in backward compatible way (#12820).

On high level, the solution can be implemented by (expanding upon ideas proposed above):

  1. adding a method to check whether all other matchers should be suppressed. If two or more matchers say "suppress all other matchers", then we could take the union of completions from those.
  2. adding priority/rank to each matcher or each completion item to be used for sorting (or sort_text if following LSP; this appears sub-optimal though and we can always convert to sortText downstream from a numeric rank)

@krassowski
Copy link
Member

krassowski commented Aug 22, 2022

On implementation level, approach (A):

(1) Suppressing can be performed as a matcher-level function. Matcher would start to resemble a proper class with methods (not just a function). We could introduce this while maintaining backward-compatibility by adding an optional should_suppress_others method via a decorator:

def matcher(*, should_suppress_others: Callable):
    def wrapper(func):
        func.should_suppress_others = should_suppress_others
        return func
    return wrapper

class IPCompleter:
    # ...
    @matcher(should_suppress_others=lambda text: does_it_look_like_I_am_in_a_dict(text))
    def dict_key_matches(self):

The new Matcher could be typed as follows:

class LegacyMatcher(Protocol):
    __call__: Callable[[str], list[str]]


class MatcherSupressionProtocol(Protocol):
    should_suppress_others: Callable[[str], bool]

Matcher = Union[LegacyMatcher, MatcherSupressionProtocol]

(2) Currently sorting is governed by hard-coded completions_sorting_key; it would be difficult to recognise a dictionary key using this existing approach alone.

Matcher-level priority/rank could be a solution if it took the request text in and return a number depending on situation (here answering "how likely it seems that we are in a dict"):

class MatcherPrioritySystemDynamic(Protocol):
    priority: Callable[[str], float]

Matcher = Union[LegacyMatcher, MatcherSupressionProtocol, MatcherPrioritySystemDynamic]

@krassowski
Copy link
Member

krassowski commented Aug 22, 2022

Completion-level priority/rank would enable more granular control for matcher developers and could expand the API proposed in my earlier comment:

class SimpleCompletion(NamedTuple):   # or TypedDict with NotRequired
    __slots__ = ('text', 'origin', 'type', 'priority')
    text: str
    origin: Optional[str]
    type: Optional[str]
    priority: Optional[float]
    

class LegacyMatcher(Protocol):
     __call__: Callable[[str], Union[list[str], list[SimpleCompletion]]]

However, what follows from my comment above is an observation that the information about priority and suppression is context-specific (both matcher.should_suppress_others(text: str) and matcher.priority(text: str) require text argument). Maybe we should reconsider whether instead of trying to gradually turn matchers into classes as suggested in approach (A), we should instead use:

Approach (B): matchers are just simple functions, but (optionally) return an extensible structure with metadata. In this scenario matchers would comply with the following typing:

class MatchingResult(TypedDict):
    matches: list[SimpleCompletion]
    suppress_others: NotRequired[bool]
    priority: NotRequired[float]

LegacyMatcher = Callable[[str], list[str]]
NewMatcher = Callable[[str], MatchingResult]   # TODO: find better code name
Matcher = Union[LegacyMatcher, NewMatcher]

Using TypedDict + NotRequired, or Protocol corresponds to implementation which will require duck typing or key checking respectively but will allow us to add more properties in the future without breaking compatibility.

@mlucool
Copy link
Author

mlucool commented Nov 7, 2022

@krassowski, thanks for the work here. A few pieces of feedback:

  1. This does not handle dicts with depth > 1:
    image

  2. As a nice to have, when you select a value, it should complete the "] as well so that I can just push enter:
    image

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.

2 participants