Skip to content

bpo-21261: IDLE - add completion of dict sting keys #15169

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 32 commits into from
Closed

bpo-21261: IDLE - add completion of dict sting keys #15169

wants to merge 32 commits into from

Conversation

taleinat
Copy link
Contributor

@taleinat taleinat commented Aug 7, 2019 •
edited by terryjreedy
Loading

This is a new implementation, created from scratch. I intentionally didn't base this on PR GH-1511 after reviewing it thoroughly.

Note that completion is intentionally limited to keys of type str and bytes only.

Additional changes made in this PR:

  • The Escape key now closes the completion window. [already done separately]
  • Completing a string or bytes dict key adds a closing square bracket.
  • Some of the auto-completion tests have been changed a bit to run without a GUI.
  • Significant refactoring and cleanup of some existing tests, with better use of mocking.
  • The fake Text widget used for tests has had its index handling greatly improved.
  • Added a very functional fake Listbox widget class.

https://bugs.python.org/issue21261

@taleinat taleinat added type-feature A feature request or enhancement needs backport to 3.7 labels Aug 7, 2019
@taleinat taleinat requested a review from terryjreedy August 7, 2019 20:33
@taleinat taleinat self-assigned this Aug 7, 2019
aeros
aeros reviewed Aug 8, 2019
View reviewed changes
aeros
aeros reviewed Aug 8, 2019
View reviewed changes
aeros
aeros approved these changes Aug 8, 2019
View reviewed changes
Copy link
Contributor

@aeros aeros left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested using the IDLE shell from the PR branch. OS: Arch Linux 5.2.3.

Test for str
Before tab:
image
After tab:
image

Test for bytes
Before tab:
image
After tab:
image

Looks like it's working as intended, nicely done! I also tested to ensure that excessively_long_key and didn't auto-complete when using d[b'e and the same for d['t -> d['thanks taleinat'. I intentionally used a space instead of an underscore for the second one (even if it goes against naming convention) to ensure there weren't any issues there. LGTM.

aeros
aeros reviewed Aug 8, 2019
View reviewed changes
aeros
aeros reviewed Aug 8, 2019
View reviewed changes
@taleinat
Copy link
Contributor Author

taleinat commented Aug 8, 2019

@aeros167, I've addressed all of the issues you brought up, plus your suggestions. I've also completed the implementation and added a ton of tests. If you have the time to give this another go, that would be great!

@taleinat
Copy link
Contributor Author

taleinat commented Aug 8, 2019

@terryjreedy, in order to have many of the tests not require a GUI, I've expanded the existing mock_tk module quite a bit, most notably adding a rather full-featured Listbox class and expanding the index handling of the Text widget to be comprehensive. Please take a look at this!

I'm a bit worried about the long-term maintenance of this, though Tkinter changes little enough that it may be negligible.

If you prefer that I roll back these additions and just have the relevant tests require having a working GUI environment, I'm fine with that too.

@aeros
Copy link
Contributor

aeros commented Aug 8, 2019

Thanks @taleinat, I'll run through the tests again later tonight and report back with the results.

aeros
aeros reviewed Aug 9, 2019
View reviewed changes
@terryjreedy
Copy link
Member

I believe the change to What's New 3.8 will prevent the auto backport to 3.7, where the file does not exist. I have usually changed What's New in separate issues, currently bpos 33821 and 33821. (That is partly because What's New has been an after thought.) Since the change to the earliest version (now WN 3.7) will backport all the way, just the change to WN 3.8 needs to be pulled to a separate PR, also attached to 21261, that is only backported to 3.8.

After 3.8.0, when What's New 3.9 is added, I will add an issue for that and 3 separate PRs will be needed to avoid manual backport.

@taleinat
Copy link
Contributor Author

I believe the change to What's New 3.8 will prevent the auto backport to 3.7, where the file does not exist.

I'm happy to execute that manual backport. It's simpler for me than having separate PRs.

decaz
decaz reviewed Aug 21, 2019
View reviewed changes
taleinat and others added 4 commits August 24, 2019 11:20
@taleinat
update comment
5f89338
@taleinat
Merge branch 'master' into idle-dict-key-completion
7a21705
@taleinat
fix completing after a bracket with no available dict key completions
c9e0307 
Also added tests for this case, as well as for typing an opening
bracket in a string, which should not trigger dict key completion.
@taleinat
add a couple more test cases
d97213d
@taleinat
Copy link
Contributor Author

I've fixed completions after opening brackets when there are no available dict key completions, e.g. [, globals()[[, {}[ and {1: 1}[. I've also ensure things work properly after typing an opening bracket ([) in a string.

At the same time, I've removed all cases where dict keys were mixed with global variables in the completion list, as requested by @terryjreedy.

@terryjreedy, that addresses all outstanding issues with this PR.

@taleinat
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

@taleinat taleinat force-pushed the idle-dict-key-completion branch from 23ee864 to 38c28f2 Compare November 14, 2019 11:08
@taleinat
Copy link
Contributor Author

I've merged in the updated master branch and removed the outdated "What's New" entries.

This is still waiting for @terryjreedy's review.

@taleinat
Merge branch 'master' into idle-dict-key-completion
530e1f7 
# Conflicts:
#	Doc/library/idle.rst
#	Lib/idlelib/autocomplete.py
#	Lib/idlelib/autocomplete_w.py
#	Lib/idlelib/idle_test/test_autocomplete.py
@taleinat
Copy link
Contributor Author

I must say that I'm especially sad that this has not made it into IDLE. :(

@terryjreedy terryjreedy added the needs backport to 3.9 only security fixes label Sep 20, 2020
@terryjreedy
Copy link
Member

I did not proper cognize until today that mixing in the tab list had been undone. Moving on...

I want to handle mock-tk stuff separately. I believe that your patch would supersede those of issue 18504. I am reserving comments and change request for that or a new issue. I will make the new PR if you want. Nothing should be removed from this patch until obsoleted by merging the new PR.

As for dict string key completion: I am surprised (and not happy) that a seemingly simple and marginally useful addition should nearly double the code in autocomplete.py. So I will be looking to see if anything can be shrunk.

I redid all the experiments discussed in my bug list with string keys and a very short delay All the lists look correct. The main issue I encountered was navigating the list by typing partial entries. I discussed this as part of post on the issue about reconsidering inclusion of bytes keys.

@terryjreedy terryjreedy changed the title bpo-21261: IDLE shell auto-completion of dict keys bpo-21261: IDLE - add completion of dict sting keys Sep 20, 2020
@terryjreedy
Copy link
Member

Changed title to reflect that string key completion works in editor (once code is run, as with imports and definitions) and upon request.

@taleinat
Copy link
Contributor Author

taleinat commented Sep 20, 2020 •
edited
Loading

As for dict string key completion: I am surprised (and not happy) that a seemingly simple and marginally useful addition should nearly double the code in autocomplete.py. So I will be looking to see if anything can be shrunk.

Indeed, I'm not convinced that including this feature is worth the extra code and effort. Removing the support for bytes would help somewhat, but the support for string keys would still be complex.

I'd feel fine if we decided not to implement this feature: If it means that the work in making this PR helped us make the decision, then it was worth it!

@taleinat taleinat mentioned this pull request May 11, 2021
Open
@taleinat
Copy link
Contributor Author

Closing in favor of a newer version of this PR, GH-26039.

@taleinat taleinat closed this May 11, 2021
@rhettinger rhettinger mentioned this pull request Apr 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@decaz decaz decaz left review comments

@terryjreedy terryjreedy terryjreedy requested changes

@aeros aeros aeros approved these changes

Assignees

@taleinat taleinat

Labels
awaiting change review needs backport to 3.9 only security fixes type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@taleinat @aeros @terryjreedy @bedevere-bot @decaz @ambv @the-knights-who-say-ni