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

IDLE: autocomplete dictionary keys #65460

Open
rhettinger opened this issue Apr 16, 2014 · 23 comments
Open

IDLE: autocomplete dictionary keys #65460

rhettinger opened this issue Apr 16, 2014 · 23 comments
Assignees
Labels
topic-IDLE type-feature A feature request or enhancement

Comments

@rhettinger
Copy link
Contributor

BPO 21261
Nosy @rhettinger, @terryjreedy, @taleinat, @vadmium, @mlouielu, @aeros
PRs
  • bpo-21261: IDLE: Add autocomplete when completing dictionary keys #1511
  • bpo-21261: IDLE - add completion of dict sting keys #15169
  • gh-65460: IDLE: add completion of dict keys of type str #26039
  • Files
  • issue21261.patch
  • issue21261.patch: With tests
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/terryjreedy'
    closed_at = None
    created_at = <Date 2014-04-16.18:21:12.627>
    labels = ['expert-IDLE', 'type-feature', '3.8', '3.9', '3.10']
    title = 'Teach IDLE to Autocomplete dictionary keys'
    updated_at = <Date 2021-05-11.20:39:47.769>
    user = 'https://github.com/rhettinger'

    bugs.python.org fields:

    activity = <Date 2021-05-11.20:39:47.769>
    actor = 'taleinat'
    assignee = 'terry.reedy'
    closed = False
    closed_date = None
    closer = None
    components = ['IDLE']
    creation = <Date 2014-04-16.18:21:12.627>
    creator = 'rhettinger'
    dependencies = []
    files = ['35700', '35709']
    hgrepos = []
    issue_num = 21261
    keywords = ['patch']
    message_count = 23.0
    messages = ['216542', '216813', '221056', '221061', '221102', '265475', '265489', '293282', '293284', '293289', '293502', '293520', '349078', '349091', '349099', '349198', '349216', '349221', '377209', '377211', '377212', '377213', '393471']
    nosy_count = 8.0
    nosy_names = ['rhettinger', 'terry.reedy', 'taleinat', 'martin.panter', 'Eduardo.Seabra', 'cdspace', 'louielu', 'aeros']
    pr_nums = ['1511', '15169', '26039']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21261'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @rhettinger
    Copy link
    Contributor Author

    IDLE can autocomplete global variable, method names, and filenames. But, it cannot complete dictionary keys.

    Here's what we want:

    >>> d = {'long_key': '10, 'short_key': 20}
    >>> d['lo<tab>

    @rhettinger rhettinger added topic-IDLE type-feature A feature request or enhancement easy labels Apr 16, 2014
    @terryjreedy
    Copy link
    Member

    Looks sensible.

    @EduardoSeabra
    Copy link
    Mannequin

    EduardoSeabra mannequin commented Jun 20, 2014

    From the example, I couldn't know if the patch should also autocomplete int and other types. So here's a patch that autocompletes string dictionary keys.
    I'm new contributing so let me know if I made anything wrong and I'll fix as soon as possible.

    @terryjreedy
    Copy link
    Member

    String keys is what Raymond requested and what looks sensible to me. A week ago, I committed test_idle/test_hyperparser.py and an incomplete test_autocomplete.py. So we can now, for this issue, at least partly follow our standard procedure of requiring tests with patches.

    I want at least a test of is_in_subscript_string(self) added to test_hyperparser. cls.code will need another line (or an existing string bracketed and the string test altered) and a new test_is_in_subscript_string added. If you are working from an installation rather that a repository, and therefore cannot write/test an addition to the new file, say so.

    The change to auto-complete is trickier. I would have to look the code versus to tests to decide what to do, if anything. I might decide to improve the autocomplete htest (human test, in repository AutoComplete.py and test_idle/htest.py) rather than the unittest.

    In any case, we need a signed contributor agreement to accept patches.
    https://www.python.org/psf/contrib
    https://www.python.org/psf/contrib/contrib-form/

    @EduardoSeabra
    Copy link
    Mannequin

    EduardoSeabra mannequin commented Jun 20, 2014

    I've added three lines to cls.code to test_hyperparser. So I can test for subscripts with double quotes, single quotes and with no strings at all.

    Should I implement try_open_completions_event for COMPLETE_DICTIONARY? Calling this event everytime someone types a string seemed a bit expensive in my opinion.

    I'm attaching the new patch.

    As fas as the signed contributor, I've already signed last week but still waiting.

    @vadmium
    Copy link
    Member

    vadmium commented May 13, 2016

    I have no idea how IDLE works internally, but I wonder if it is possible to share some of the work with the Readline completer (rlcompleter module). Despite the name, rlcompleter should be usable without also using Readline, though recently (bpo-25660) I think this was broken. Anyway, maybe see bpo-10351 for the beginning of an rlcompleter patch and some potential test cases.

    @terryjreedy
    Copy link
    Member

    Thanks. Being on Windows, I never paid attention to rlcompleter. The big difference is that IDLE completions use IDLE's hyperparser module (which is used for other purposes also). I will look at the tests.

    @mlouielu
    Copy link
    Mannequin

    mlouielu mannequin commented May 9, 2017

    In this PR, it will complete dictionary key with string, int, and others.

    for example:
    
        d = {'long_key': 10, 'short_key': 20, 30: 40, (((1, 2), 3, 4), 5): 50}
        d['lo<tab>  -> d['long_key'
        d[(((1<tab> -> d[(((1, 2), 3, 4), 5)
        d[3<tab>    -> d[30

    The problem is, autocomplete_w can't figure the original key is string or others, so this will be possible:

    d[long<tab> -> d[long_key]
    d[shor<tab> -> d[short_key]
    

    @rhettinger
    Copy link
    Contributor Author

    Would it be safer/simpler to just autocomplete string keys.

    @mlouielu
    Copy link
    Mannequin

    mlouielu mannequin commented May 9, 2017

    I'm not sure the "safer" meaning. If it is about for beginner less confuse when mistakenly typing "d[long_<tab>", the answer will be yes for only complete string keys.

    Impl complexity between str-only and not-str-only will not have too much different, only need to change the sentinel and some other work.

    @rhettinger
    Copy link
    Contributor Author

    I think it would be unusual for tab completion to work on non-strings and would create a weird feel to the API.

    @terryjreedy
    Copy link
    Member

    IDLE currently completes global names, attributes after ., and filename segments after / or \ within a string. In the later two cases, a box will pop up automatically after a user selected time after typing . or /\ and nothing thereafter. The filename segments are not quoted in the list box.

    These completions work within subscripts.
    d[a<tab or wait> pops up global name completion box
    d['/<tab or wait> pops up filename completion box

    Raymond proposes that IDLE complete 'dictionary [string] keys'. To properly code and test, we need a more complete specification. For instance, "a string key box should open after an opening quote that follows '[' that follows a dict expression". Any opening quote should work, just as for filename completion.

    This is similar "a calltip opens after a '(' that follows a callable expresssion". For calltips, the expression cannot contain a function call, because calls can take an indeterminant amount of time. If "expression.find('(') != -1", the calltip is aborted and the same should be true here. Also, calltips.get_entity(expression) should be reused to get the dict object. (test_calltips should but does not test that 'f()(' is ignored and get_entity not called. The same should be true for "f()['".)

    Nice (?) but not necessary: delayed auto-popup after typing "d[<open quote>". This seems that it would be more difficult than the current auto popups. And see the following.

    This proposal conflicts with filename completion for subscripts. When one is accessing an existing value, one would want key completion. If one is assigning a value to a new filename key, one would want filename completion. The simplest solution I can think of is to not auto pop up key completion but to require <tab> before typing (/\) and waiting.

    Lastly, should the string keys be quoted in the box?
    | long key |
    | short key |
    or
    |'long key' |
    |'short key'|
    ?

    Selecting key objects by their representation is tempting, but it is conceptually different from completing names. Objects may have one canonical representation, but many possible representations. So clicking on a list (which currently does not work) or using movement keys is more sensible than typing chars that have to match one of many possibilities. String keys would have to be quoted.

    So I would only consider this as a separate issue, depending on a fix for clicks. It should only be accessed by <tab> immediately after '[', and I might want to disable selection by character matching.

    Even then, I would be dubious. I grepped idlelib for "\w\[". A majority of subscripts are names, handled by current name completion or not (if the names are local, which they often are). The rest are either list indexes and slices involving literal ints or string keys, which this proposal would handle for accessible dicts. I am pretty sure there are no keys other than names and strings.

    But the sparsity of use cases is my problem even with this proposal. Calltips are useful because there are many globally accessible callables, including builtins and imports. But other than class __dicts__, there are few globally accessible dicts, except perhaps in toy beginner code. Raymond, have I missed something?

    The idlelib grep had 763 hits and I believe more that half are for dicts. But they may all be locals or self attributes. I would love to be able, for instance, to type "local_dict['<tab>" and fill in 'background', but that will not work.

    @terryjreedy terryjreedy added the 3.7 (EOL) end of life label May 12, 2017
    @taleinat
    Copy link
    Contributor

    taleinat commented Aug 5, 2019

    I had no idea that this was desired... I had this working in a old version of autocomplete back before 2010! I'm not sure whether I'll be able to find it though.

    I can't understand why Louie's PR was closed, it seemed to be going in the right direction... Any explanation?

    @taleinat taleinat added 3.8 only security fixes 3.9 only security fixes labels Aug 5, 2019
    @terryjreedy
    Copy link
    Member

    The history is confusing. bpo user Louie Lu (louielu), as github user 'noname louisom', open PR 1511 for this existing issue on 2017 May 9. On May 12, he opened bpo-30348 and PR 1512 with the tests for fetch_completions and get_entity that were part of PR 1511. (This was a needed separation.)

    By June, he had switched to a new github name 'Louie Lu mlouielu'. On June 12, he opened bpo-30632 and PR 2124, which duplicated bpo-30348 and PR 1512 (which partly duplicated PR 1511). On June 15, he closed PR 1511 to 'migrate' it to pr 2209. But the latter only included the tests also in PR 1512, which it replaced on bpo-30348, and PR 2124. He also closed bpo-1512. Loiue moved on to other projects in Fall, 2017.

    After revisions, I merged PR 2209 for bpo-30348 last March. I followed up with bpo-36419, PR 15121, now merged. I just opened bpo-37766 to finish preparing autocomplete for new additions such as this. I was thinking of this issue when I included adding an htest. (Note: bpo-27609 is the master issue for completions.)

    Notes for migrating the dict keys code:

    1. In PR 15121, I shrank mode names to ATTRS and FILES. The new mode name should be KEYS, or maybe SKEYS (for string keys). Other refactors should not affect KEYS too much.
    2. I intend to change 'smalll' and 'bigl' to 'small' and 'big' and might make other changes to fetch_completions.
    3. I intend to split test_fetch_completions into separate methods for each mode. The new KEYS tests should be a separate method 'test_fetch_keys'.

    The questions of function calls in the entity expression is more nuanced than I know when I wrote msg293520. For force-open-completions, control-space, function calls are allowed. But I do not think that this affects the new mode.

    @terryjreedy terryjreedy self-assigned this Aug 5, 2019
    @rhettinger
    Copy link
    Contributor Author

    Thanks for going through the history of this issue. It was surprisingly convoluted. I still hope this feature comes to fruition.

    @taleinat
    Copy link
    Contributor

    taleinat commented Aug 7, 2019

    Raymond, your with may just come true!

    I've just created PR #59374 with a new implementation of my own, complete with tests. I have not yet thoroughly tested it though, and would like some feedback on it.

    @aeros
    Copy link
    Contributor

    aeros commented Aug 8, 2019

    I have not yet thoroughly tested it though, and would like some feedback on it.

    I performed some testing on Linux and it looks good as far as I can tell. I added a few minor suggestions, but the auto-complete seems to be functioning as desired.

    @taleinat
    Copy link
    Contributor

    taleinat commented Aug 8, 2019

    Many thanks for the review, Kyle!

    @terryjreedy
    Copy link
    Member

    The current patch completes both strings keys and bytes keys mixed together. I want to challenge this.

    1. What is the use case? The current patch nearly doubles the autocomple code, which handle user actions up to creation of a completion list. I would prefer smaller.

    2. We agreed on completing 'string keys'. Bytes are not strings. Bytes might represent an encoded string, but might instead be anything else. Some bytes are represented by ascii chars, but the majority need hex escapes.

    3. Including bytes forces quoting keys in the list box in order to add the 'b' prefix for bytes. I said above that we should quote anyway, but that was before working with the implementation and discovering the resulting usage issues.

    3. Including bytes makes it harder to select a key by typing a partial key.  Suppose d had hundreds of key, a to z.  One want to select 'zeta13  c3'.
    >>> d[ <pause> opens a list box with starting with the 'a' words.
    One enters 'z'.  Nothing happens because no entry starts with 'a'.
    Backspace, enter "'z".  Nothings happens because the default quote used in the listbox is '"'.  If 'b' did not possibly represent a bytes prefix, we could either remove surrounding quotes or ignore them when matching.

    @terryjreedy terryjreedy added 3.10 only security fixes and removed 3.7 (EOL) end of life labels Sep 20, 2020
    @taleinat
    Copy link
    Contributor

    People still get confused between bytes and strings, even with Python 3, especially novice programmers who are a major target audience of IDLE.

    I simply thought it would be confusing if string dict keys did appear in the completion list, but bytes keys did not.

    You're certainly right that removing support for completing bytes keys would simplify the code.

    @taleinat
    Copy link
    Contributor

    Also, please note that the PR allows using either type of quote when typing completions.

    @terryjreedy
    Copy link
    Member

    the PR allows using either type of quote when typing completions.

    Only if one types a quote before the box pops up. In this case, there is no auto popup and one must request completions. The box then uses the quote typed. Thereafter, there is no choice. Backspacing to delete the quote and typing the another one disables matching.

    If one type >>> d[ and pauses long enough for the auto-popup, (I have 'wait' set to 200 milleseconds), one must similarly match the existing quote, because jumping ahead requires a string match.  Because of the bytes inclusion, the match much include the opening quote, because otherwise an initial 'b' would be ambiguous.  Without bytes, we could either removed the quotes from the box or automatically add an open quote.

    @taleinat
    Copy link
    Contributor

    Note that I've created a new version of the latest PR, with support for dict keys of type bytes removed.

    @taleinat taleinat removed the easy label May 11, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @terryjreedy terryjreedy changed the title Teach IDLE to Autocomplete dictionary keys IDLE: autocomplete dictionary keys Oct 5, 2022
    @terryjreedy terryjreedy removed 3.10 only security fixes 3.9 only security fixes 3.8 only security fixes labels Oct 5, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-IDLE type-feature A feature request or enhancement
    Projects
    Status: In Progress
    Development

    No branches or pull requests

    5 participants