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

PR: Make a closebrackets extension for smarter brackets #8637

Merged
merged 6 commits into from Jan 29, 2019

Conversation

bcolsen
Copy link
Member

@bcolsen bcolsen commented Jan 24, 2019

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot (if affecting the UI)

I made a CloseBrackets extension to the editor like the CloseQuotes extension

Issue(s) Resolved

Fixes #3414

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: bcolsen

@pep8speaks
Copy link

pep8speaks commented Jan 24, 2019

Hello @bcolsen! Thanks for updating the PR.

Line 67:23: E128 continuation line under-indented for visual indent

Comment last updated on January 29, 2019 at 06:53 Hours UTC

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Seemed to work well in my basic functional testing so far! Just some minor style-related comments from me, aside from the remaining PEP 8 warnings.

Great addition @bcolsen ! Really useful feature.

@ccordoba12
Copy link
Member

Hey @bcolsen, is this ready for review? (It looks like it is).

@bcolsen bcolsen changed the title [WIP] Make a closebrackets extension for smarter brackets PR: Make a closebrackets extension for smarter brackets Jan 28, 2019
@bcolsen
Copy link
Member Author

bcolsen commented Jan 28, 2019

@ccordoba12 Yes it ready, I forgot to change the title.

@ccordoba12
Copy link
Member

@bcolsen, everything looks great! I just have one question before merging: will selected text be enclosed by default in braces and quotes? Or do users have to activate an option in our preferences for that?

@bcolsen
Copy link
Member Author

bcolsen commented Jan 28, 2019

The current be behavior of closebrackets is that the entire extension isn't loaded when the option "Automatic insertion of parenthesis, brackets and braces" is unchecked. This was done to mirror the behavior of the closequotes. I could see that people would want to disable both of those separately, but it that could be done in a separate issue.

@CAM-Gerlach
Copy link
Member

I could see that people would want to disable both of those separately, but it that could be done in a separate issue.

Yeah, I was thinking about that too, but I didn't want to overcomplicate this issue or break consistency with the auto quotes option. If we do separate them, I'd suggest adding surrounding characters (quotes + brackets) when text is selected be one separate option (since the usecase for the two is similar and the downsides are minimal), while keeping the insertion of the closing char on typing the opening one without anything selected be the two options they are now (since the use case/preference at least somewhat differs between them).

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM now!

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @bcolsen! Great work here!!

@ccordoba12 ccordoba12 merged commit 4d7e092 into spyder-ide:master Jan 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants