-
-
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: Make the switcher a plugin #20714
PR: Make the switcher a plugin #20714
Conversation
The methods inside the plugin class may be reorganized
because it's refering to a non existing widget (main.switcher)
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.
Hey @angelasofiaremolinagutierrez, your code looks really good and almost ready to be merged. Thanks for your work on it!
Most of my suggestions are purely stylistic. In case you haven't worked with Github suggestions before, please take a look at this page to understand how to apply them in a single commit.
Besides my comments above, you also need to add https://github.com/spyder-ide/spyder/blob/master/spyder/plugins/plots/__init__.py That way Python will recognize those directories as proper modules. |
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Fixes missing from first time Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
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.
Thank you for all the work here @angelasofiaremolinagutierrez ! Left some suggestions to hopefully fix the failing tests. The suggestions mainly focus on:
- Some extra code that got readded and should be removed related to editor actions that were being done inside the mainwindow but that are not there anymore (
create_edit_action
related things). - Some extra changes in the config related files to make the switcher shortcuts work.
- A way to keep the switcher working by creating the
EditorSwitcherManager
instance - Some code style things (extra readded comments to remove, a white space, etc).
If you have any question regarding the suggestions let me know 👍
Standardize code style and remove code that was re-added from resolving conflict Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
add switcher to context extra valid list
Closing and reopening to restart our tests. |
These functions were previously removed because they were only used in tests that used to be skipped. Now that they are not skipped they are added again.
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.
After checking for some time I think I found a way to workaround the issue with the sections order in the File
menu @angelasofiaremolinagutierrez @ccordoba12 !
Left some comments to address that. Also, left some code style suggestions that I found while checking the code.
Finally, checked locally and seems like everything is working as expected 🎉, so other than the above this LGTM 👍
Co-authored-by: Daniel Althviz Moré <d.althviz10@uniandes.edu.co>
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.
Looks good to me now. Thanks @angelasofiaremolinagutierrez for your great work here!
Description of Changes
EditorSwitcherManager
and the methods that this manager handles are not migrated yet.What is left to do
EditorSwitcherManager
before_section
parameter in each section, but it didn't work. There might be a bug in the way the menus are getting ordered:spyder/spyder/api/widgets/menus.py
Line 220 in ad14a7b
Issue(s) Resolved
This is necessary to solve #3860
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: @angelasofiaremolinagutierrez