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: Add methods to simplify access to our config system to MainWindow #18911

Merged
merged 6 commits into from
Aug 5, 2022

Conversation

ccordoba12
Copy link
Member

@ccordoba12 ccordoba12 commented Aug 3, 2022

Description of Changes

  • Small refactoring whose main purpose is to add get_conf/set_conf methods to MainWindow in order to simplify how we access config options on that class.
  • Move __init__ to be at the beginning of MainWindow (I think it's odd to have it in the middle of the class).
  • Move the Qt methods we override on MainWindow to be on their own section.
  • Add more section separators to MainWindow to make easier to navigate it.
  • Remove old Mac stylesheet because it's no longer needed.

Notes:

  1. I did these changes while working to fix another bug. I preferred to break that into two parts to facilitate reviewing it.
  2. I think we should avoid using CONF because it's cleaner and easier to read to have methods that call it behind the scenes.

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: @ccordoba12

@ccordoba12 ccordoba12 added this to the v5.3.3 milestone Aug 3, 2022
@ccordoba12 ccordoba12 self-assigned this Aug 3, 2022
@pep8speaks
Copy link

pep8speaks commented Aug 3, 2022

Hello @ccordoba12! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 83:1: E402 module level import not at top of file

Comment last updated at 2022-08-05 18:24:37 UTC

This is no longer used, so there's no need to keep it.
@ccordoba12
Copy link
Member Author

ccordoba12 commented Aug 3, 2022

Let's hold this PR until PR #18778 is merged because it touches the same code.

@ccordoba12
Copy link
Member Author

@dalthviz, scratch my last comment because Ryan agreed to leave PR #18778 for Spyder 6.

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @ccordoba12 ! Left a comment regarding the creation of a method to access shortcuts in a similar manner as to get and set configurations. Other than that this LGTM 👍

spyder/app/mainwindow.py Outdated Show resolved Hide resolved
Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

👍

@ccordoba12 ccordoba12 merged commit fac0b38 into spyder-ide:5.x Aug 5, 2022
@ccordoba12 ccordoba12 deleted the simplify-main-window-conf branch August 5, 2022 21:11
ccordoba12 added a commit that referenced this pull request Aug 5, 2022
@ccordoba12 ccordoba12 changed the title PR: Add methods to simplify access to our config system in MainWindow PR: Add methods to simplify access to our config system to MainWindow Aug 5, 2022
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

3 participants