-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Add Editor extensions #5002
Conversation
Hello @rlaverde! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on August 23, 2017 at 16:01 Hours UTC |
@@ -247,6 +249,8 @@ class CodeEditor(TextEditBaseWidget): | |||
#: Signal emitted when a key is pressed | |||
key_pressed = Signal(QKeyEvent) | |||
|
|||
sig_mouse_pressed = Signal(QMouseEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem necessary as part of this PR, right?
I really like this, it's very neat! I just have one suggestion: could we add as part of this PR an That way it'd be easier to understand how to use extensions for the rest of us ;-) |
3bb3286
to
da9e752
Compare
…tor helper functions public.
da9e752
to
06a1e12
Compare
@ccordoba12 This is ready, I moved close quote logic to an editor extension, and write some tests, although I didn't found any errors with that code. Should I enable close quote by default? |
return '' | ||
|
||
|
||
class QuoteEditorExtension(EditorExtension): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this CloseQuoteExtension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, CloseQuotesExtension
I think issue #877 has several examples. Did you look at it?
If tests pass, why not? ;-) |
|
||
def __init__(self, editor): | ||
"""Initialize and add a reference to the editor.""" | ||
super(EditorExtensionsManager, self).__init__(editor) | ||
self._extensions = {} | ||
|
||
def append(self, extension): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add/remove are the antonyms, not append/remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think append is more list-pythonic, should I change it for add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is, but append even your docsting uses ADD and not APPEND, so you can see why append is not really a good choice, independent of the structure used to store extensions (dict, list, set or whatever). So either we append/prepend or add/remove, insert/pop, but not a mix of those. So yes, please change to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't see your response because It was collapsed, I'm going to change it for add
""" | ||
This module contains the close quotes editor extension | ||
""" | ||
"""This module contains the close quotes editor extension.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just use extensions
folder? like spyder/widgets/sourcecode/extensions/closequotes.py
7198526
to
c96352f
Compare
I agree with @goanpeca to use only the |
Yes, me too (at first I thought that editorextensions was more specific, anyway they are into the |
I implement #877 (comment) although I change the behavior, It'll just keep the text selected allowing to add as many quotes as you like. The parenthesis are managed by other logic, so integrate it with this extension could break some of their functionality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terrific job @rlaverde! I really like this!
Ok, no problem. If more issues appear in the future, now we can fix and add tests for this quite easily. |
Fixes: #877
See #4987 (comment)
I use #877 as example of an editor extension
They idea is to create an extension in
spyder/widgets/sourcecode/extensions/
with a class that Inherits oEditorExtension
(api.editor.extension), create custom methods and connect them in theon_uninstall
method, and the add them to theCodeeditor.editor_extensions
There is already some signals for key press, and paint event,
I also added a signal for mouse pressed.Maybe there is missing a signal for timerEvent.ping @jnsebgosselin