Skip to content

Brackets completion #934

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

Merged
merged 31 commits into from
Nov 9, 2021

Conversation

samuelgregorovic
Copy link
Contributor

@samuelgregorovic samuelgregorovic commented Oct 30, 2021

Solution for #905

As requested, posted here for feedback (still needs polishing, more docs and tests).

Changes include traditional IDEs behavior on completing brackets ( [ { and quotes " '.
This works in writing, as well as with callable autocomplete matches.

Example:
If you type in def foo(, the line will print def foo() and put the cursor between the brackets. This also works with nested brackets and quotes. So if you do x = 5*(some_list[5]), the brackets will close upon writing the starting paren/quote, and put your cursor inside them.

I also implemented behavior on BACKSPACE key and TAB key.
If your cursor is currently positioned right on the character, which is a closing bracket or a quote and you hit TAB, your cursor will move behind that symbol.
If you hit BACKSPACE and the first character to be deleted is an opening bracket, and your cursor is on a closing bracket, this will delete both brackets/quotes.
This behavior also works in nested brackets environment.

Feature can be enabled in config file by setting the brackets_completion flag to True.

Thank you for the feedback.

Still needs tests. Guidance or advice appreciated.

@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2021

Codecov Report

Merging #934 (2c683fc) into main (18f6901) will increase coverage by 0.71%.
The diff coverage is 94.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #934      +/-   ##
==========================================
+ Coverage   67.98%   68.69%   +0.71%     
==========================================
  Files          61       62       +1     
  Lines        9174     9437     +263     
==========================================
+ Hits         6237     6483     +246     
- Misses       2937     2954      +17     
Impacted Files Coverage Δ
bpython/config.py 87.83% <ø> (+0.08%) ⬆️
bpython/curtsiesfrontend/repl.py 65.83% <85.45%> (+0.77%) ⬆️
bpython/line.py 95.86% <92.85%> (-0.33%) ⬇️
bpython/test/test_brackets_completion.py 99.13% <99.13%> (ø)
bpython/curtsiesfrontend/manual_readline.py 90.15% <100.00%> (+0.20%) ⬆️
bpython/urwid.py 0.00% <0.00%> (ø)
bpython/test/test_curtsies_repl.py 96.11% <0.00%> (ø)
bpython/test/test_importcompletion.py 92.00% <0.00%> (ø)
bpython/curtsiesfrontend/interpreter.py 100.00% <0.00%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18f6901...2c683fc. Read the comment docs.

@thomasballinger
Copy link
Member

I just played with this a bit. Since this would be an optional feature, these things wouldn't be required from my perspective (maybe none of them are worth the complexity!) but might be nice.

Overwriting closing characters

This already works for ) and ], but I'm missing it for strings too.

>>> s = |
>>> s = '|' # after pressing '
>>> s = 'a|'  # after pressing a
>>> s = 'a|'''  # after pressing '

To avoid the case above, a closing character could overwrite an existing one in some cases. This might be tricky to implement but it is behavior that makes autoclosing pairs in IDEs manageable for me, because I can still type what I normally would. Always overwriting any closing characters, ignoring whether they were automatically added, sounds easier.

Don't autoclose when cursor is followed by non-whitespace

I often forget that a socket.socket's connect method takes a single argument, a tuple:

>>> s = s.connect('localhost', 8080|)
>>> s = s.connect('localhost', |8080)  # back word
>>> s = s.connect(|'localhost', 8080)  # back a bunch more
>>> s = s.connect((|)'localhost', 8080)  # after pressing (

To avoid this, autoclosing could take place only when the the cursor is at the end of a line? I notice VSCode uses a fancier heuristic than this, but it seems based on what is to the right of the cursor. If it's a space or nothing, it seems to auto-close.

Remove autoclose when open is deleted (by method other than backspace)

This already works for backspace, but not some other ways of clearing the line:

>>> x = |
>>> x = (|)  # after pressing (
>>> x = (1|)  # after pressing 1
>>> x = |)  # after pressing ctrl-w

>>> x = |
>>> x = (|)  # after pressing (
>>> |)  # after pressing ctrl-u

VSCode has the same behavior as this PR, so I can hardly complain — but there are series of keystrokes I make similar to backspace that delete the opening character without deleting the matching close character.

Matching history entries

A closing paren causes history entries to no longer match.

>>> def foo(a, b, c):
...     pass
>>>

>>> d|   # after pressing d, fish-style history shows the previously typed line in grey
>>> def foo(|)  # after pressing (, the `def foo(a, b, c):` line from history is no longer suggested

Not a big issue, but the the closing paren can cause lines from history to no longer match. Maybe it could be removed when searching through history.

@samuelgregorovic
Copy link
Contributor Author

@thomasballinger i will read through this and get back to you

@samuelgregorovic
Copy link
Contributor Author

samuelgregorovic commented Nov 2, 2021

Overwriting closing characters

This already works for ) and ], but I'm missing it for strings too.

>>> s = |
>>> s = '|' # after pressing '
>>> s = 'a|'  # after pressing a
>>> s = 'a|'''  # after pressing '

To avoid the case above, a closing character could overwrite an existing one in some cases. This might be tricky to implement but it is behavior that makes autoclosing pairs in IDEs manageable for me, because I can still type what I normally would. Always overwriting any closing characters, ignoring whether they were automatically added, sounds easier.

  • this issue is present because this part of a "switch" in repl.py on line 793:
...
elif e in CHARACTER_PAIR_MAP.keys():
    self.insert_char_pair_start(e)
elif e in CHARACTER_PAIR_MAP.values():
    self.insert_char_pair_end(e)
...

The problem is that opening and closing characters are the same and are difficult to distinguish. i will look into it and come up with a solution. BTW, tabbing still works even with quotes.

Don't autoclose when cursor is followed by non-whitespace

  • this shouldnt be a problem to implement

Remove autoclose when open is deleted (by method other than backspace)

  • this one is difficult to decide. i am also used to VS code behaviour and it does this only on backspace action. not even DEL key removes the closing bracket. i think we should leave it as is IMO. what do you think?

Matching history entries

  • shouldnt be a problem either. i can print the closing paren to the line but omit it afterwards

These changes shouldnt take a lot of time.

@thomasballinger
Copy link
Member

BTW, tabbing still works even with quotes.

great, that's good

this one is difficult to decide. i am also used to VS code behaviour and it does this only on backspace action. not even DEL key removes the closing bracket. i think we should leave it as is IMO. what do you think?

Sounds fine to me, I'm not sure how I'd go about this either.

shouldnt be a problem either. i can print the closing paren to the line but omit it afterwards

Sounds good, but if it's very complicated feel free to skip this one — it's something that I noticed, but I'm not sure it's worth fixing.

@samuelgregorovic
Copy link
Contributor Author

samuelgregorovic commented Nov 2, 2021

@thomasballinger

Don't autoclose when cursor is followed by non-whitespace

I often forget that a socket.socket's connect method takes a single argument, a tuple:

>>> s = s.connect('localhost', 8080|)
>>> s = s.connect('localhost', |8080)  # back word
>>> s = s.connect(|'localhost', 8080)  # back a bunch more
>>> s = s.connect((|)'localhost', 8080)  # after pressing (

To avoid this, autoclosing could take place only when the the cursor is at the end of a line? I notice VSCode uses a fancier heuristic than this, but it seems based on what is to the right of the cursor. If it's a space or nothing, it seems to auto-close.

i just realized, when testing the solution for this problem, that it breaks another good behaviour. if you write multiple nested open parens (|) you expect that you will be moved inside them ((|)). however if we limit the autoclosing by the following character (in this case ), which is not end of line or space, it effectively destroys this feature. it becomes ((|). we could maybe take all closing brackets, excluding quotes, as allowed chars (right of the cursor) when autocompleting. thoughts?

@thomasballinger
Copy link
Member

thomasballinger commented Nov 2, 2021

Hm, maybe the behavior I think I want is too complex to be worth implementing. What I see in VSCode (which I think does what I want, but I'm not even sure about that — it does work for this one socket case) is:

   ( ( ( ) ) )   # ignore the spaces
  | | |          # adding a left paren ( here does not autoinsert a right paren )
        | | | |  # adding a left paren ( here does autoinsert a right paren )

@samuelgregorovic
Copy link
Contributor Author

samuelgregorovic commented Nov 2, 2021

@thomasballinger thats exactly what i meant by previous comment! (pseudocode):

if next_char in [ ')', ']', '}' , ' ']: # or end of line
    complete_bracket()
else:
    pass

this way, the case with the socket works. basically it will autocomplete very similarly to VS Code
EDIT: see latest commit

@samuelgregorovic
Copy link
Contributor Author

samuelgregorovic commented Nov 2, 2021

Matching history entries

A closing paren causes history entries to no longer match.

>>> def foo(a, b, c):
...     pass
>>>

>>> d|   # after pressing d, fish-style history shows the previously typed line in grey
>>> def foo(|)  # after pressing (, the `def foo(a, b, c):` line from history is no longer suggested

Not a big issue, but the the closing paren can cause lines from history to no longer match. Maybe it could be removed when searching through history.

i looked into the missing history hints, and basically the problem seems to be that the add_normal_character method called cursor_offset setter, which called _set_cursor_offset method with update_completion default param set to True this was triggering the unwanted update. should be okay now. i also moved the bracket completion to the end of the method on_tab, because it was triggering before the search completion and did override it. try it out, should be better now.

@samuelgregorovic
Copy link
Contributor Author

@thomasballinger
i can try to write some tests for batch of different scenarios. do you have some guidelines concerning that? should i create a new file or put it elsewhere? thanks!

@thomasballinger
Copy link
Member

@thomasballinger i can try to write some tests for batch of different scenarios. do you have some guidelines concerning that? should i create a new file or put it elsewhere? thanks!

Whatever is convenient for you! A new file sounds fine to me. If not a new file, bpython/test/test_curtsies_repl.py might be the right file.

@samuelgregorovic
Copy link
Contributor Author

@thomasballinger i wrote test for scenarios we talked about, including quotes and all brackets with TAB, BACKSPACE and others. i just cant figure out how to write a test which would include the search history. any advice, pointer? thanks!

Copy link
Member

@thomasballinger thomasballinger left a comment

Choose a reason for hiding this comment

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

Hm, I'll take a look at the history thing.

@thomasballinger
Copy link
Member

I think you can check the value of self.rl_history.saved_line? If it's not populated, may need to call self.rl_history.enter('def foo(a, b, c):') to populate it.

@thomasballinger
Copy link
Member

Oh if you're checking for the grayed out text completion you might need to set self.config.curtsies_right_arrow_completion

@samuelgregorovic
Copy link
Contributor Author

thank you! any other ideas to add to this feature?

@thomasballinger
Copy link
Member

Seems good to me! I'm looking forward to it. option-r reverse history search is the only thing that I think is left to change.

@thomasballinger
Copy link
Member

This looks good to me! I'll wait for CI to run then merge it.

@thomasballinger thomasballinger merged commit 3ba6e16 into bpython:main Nov 9, 2021
@samuelgregorovic
Copy link
Contributor Author

This looks good to me! I'll wait for CI to run then merge it.

great! hopefuly it will serve the users good

@yogeshdofficial
Copy link

hey I am the one who opened this enhancement thanks for merging this
but I edited my config but I doesn't work,Is it not available for the version installed from pip?
Can you guide me how to get the feauture you implemented?

edit:I am new to python and english

@thomasballinger
Copy link
Member

thomasballinger commented Nov 11, 2021

@yogeshdcool I didn't merge this feature in in time for the release that came out a few days ago, so you'll either need to wait for the next release or install bpython from source. Here's how to do that with pip:

python -m pip install git+https://github.com/bpython/bpython.git@main

@yogeshdofficial
Copy link

Thanks for helping me out

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.

5 participants