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 the internal console use the same theme as the other widgets #8251

Merged
merged 12 commits into from
Nov 22, 2018

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Nov 15, 2018

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based this PR on the latest version of the correct branch (master or 3.x)
  • Described the changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed
  • Included a screenshot, if this PR makes any visible changes to the UI

Developer Certificate of Origin Affirmation

By submitting this Pull Request or typing my name below, I affirm the
Developer Certificate of Origin
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: dalthviz

Description of Changes

  • Add icons for regex, case sensitive and message box information that support the dark theme
  • Internal console dark theme support

Issue(s) Resolved

Fixes #8072
Part of #8068

@pep8speaks
Copy link

pep8speaks commented Nov 15, 2018

Hello @dalthviz! Thanks for updating the PR.

Line 33:80: E501 line too long (87 > 79 characters)

Comment last updated on November 22, 2018 at 19:43 Hours UTC

@ccordoba12
Copy link
Member

ccordoba12 commented Nov 15, 2018

@dalthviz, please remove regexp.svg and the note about it in https://github.com/spyder-ide/spyder/blob/master/NOTICE.txt

Also, please remove upper_lower.png and its mention in NOTICE.txt

@goanpeca
Copy link
Member

Screenshot :-p ?

@dalthviz
Copy link
Member Author

@ccordoba12 @goanpeca a preview:

Find/Replace:

imagen

Find in Files:

imagen

MessageboxInformation:

info

Internal Console:

imagen

@dalthviz dalthviz changed the title [WIP] PR: dark icons for regex, case sensitive and message box information PR: dark icons for regex, case sensitive and message box information Nov 18, 2018
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.

Essentially two main comments: We should IMO require QDarkStyle 2.6.3, and the hanging indents were all over the place (some 8 spaces, some 16, while PEP 8 specifies 4 except in function defs (Spyder's automatic indent behavior here is incorrect).

README.md Outdated Show resolved Hide resolved
requirements/requirements.txt Outdated Show resolved Hide resolved
spyder/plugins/console/plugin.py Outdated Show resolved Hide resolved
spyder/plugins/console/widgets/internalshell.py Outdated Show resolved Hide resolved
spyder/plugins/console/widgets/shell.py Outdated Show resolved Hide resolved
spyder/plugins/console/widgets/shell.py Outdated Show resolved Hide resolved
spyder/widgets/findreplace.py Outdated Show resolved Hide resolved
spyder/widgets/reporterror.py Outdated Show resolved Hide resolved
spyder/widgets/reporterror.py Outdated Show resolved Hide resolved
@jitseniesen
Copy link
Member

while PEP 8 specifies 4 [spaces for hanging indents] except in function defs

That is not my understanding from https://www.python.org/dev/peps/pep-0008/#indentation and in particular "The 4-space rule is optional for continuation lines." Nevertheless, I usually prefer 4 spaces.

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Nov 19, 2018

while PEP 8 specifies 4 [spaces for hanging indents] except in function defs

That is not my understanding from https://www.python.org/dev/peps/pep-0008/#indentation and in particular "The 4-space rule is optional for continuation lines." Nevertheless, I usually prefer 4 spaces.

That is my understanding also. PEP 8 is suggesting, not specifying to use 4 spaces for hanging indents.

image

But, in our wiki (https://github.com/spyder-ide/spyder/wiki/Dev:-Coding-Style#python-style), we clearly state to "Always use four (4) spaces for indentation."

So I suggest that we update this section every time a coding style issue arise in one of our PR and stick to it.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 19, 2018

while PEP 8 specifies 4 [spaces for hanging indents] except in function defs

That is my understanding also. PEP 8 is suggesting, not specifying to use 4 spaces for hanging indents.

I mis-remembered that it was at the recommendation, not suggestion level but you are clearly correct. My mistaken wording, sorry. Ultimately, the justification for the change was that 4 spaces was what we universally use everywhere else for continuation lines, and therefore we should be consistent.

So I suggest that we update this section every time a coding style issue arise in one of our PR and stick to it.

Great suggestion! For starters I can add what @ccordoba12 decided about not following the "one-line summary" rule for docstrings in the tests.

We can also add a "Project Standards" section in the Contributing Guide with a bullet point summary and then links to that wiki page for more information.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 19, 2018

Responding to @dalthviz 's comment above:

        ShellBaseWidget.__init__(
                        self, parent, history_filename, profile=profile,

Hanging indent—is there a reason for the apparently non-standard positioning I'm missing?

Just a personal preference. I normally left the indent in a way I find easier to read the begin and end of the parameters while using hanging indent (without using a specific number of spaces)

PEP 8 is meant to be broken if its clear that doing something different is optimal for a specific case, and it is broken consistently.

The problem, however, arises when one's own personal preference is not the same as everybody else's personal preference in a large project like Spyder, leading to both inconsistency (both de-jure, with our coding guidelines, and de-facto, with most if not all of the other new code in Spyder), and being more difficult to understand, never mind apply to one's own code, for anyone else not used to it. Personally, I find it confusing and harder to grasp at a glance myself; that may be because I'm not used to it but that's exactly my point). In fact, even in this PR it isn't done consistently, as several others I pointed out have 4 or 8 spaces.

Furthermore, this isn't even really a hanging indent at all, since by the definitions I've seen, a hanging indent is necessarily some fixed number >0 of spaces right of the start of the previous line or the margin, whereas this indent, like that lining up with the parens, is indexed to a particular character in the above. There's no particular name for it, since it doesn't exist at all in PEP 8. Finally, it due to the choice of indent (lining up with the ClassName. directly implies that all of the parameters of __init__ are properties of ClassName, which isn't necessarily true for any arbitrary parameter of __init__ (they may not be set as properties, or at least not by their argument names, etc).

IMO, unless we all agree we like this style, will follow this style in all new code, and will instruct new contributors on this style, we should stick with a consistent 4-space hanging indent (or at least a n-space hanging indent with some consistent number of spaces, not...whatever this is, since its not even really a hanging indent at all).

@jnsebgosselin @jitseniesen Your opinions, since we just discussed this below?

@jnsebgosselin
Copy link
Member

@jnsebgosselin @jitseniesen Your opinions, since we just discussed this below?

It's not my call, but I can still give my opinion :)

I'm in favor of using 4-space hanging indents everywhere. It's clear, easy to follow and ensure consistency throughout the code.

@CAM-Gerlach
Copy link
Member

I'm in favor of using 4-space hanging indents everywhere. It's clear, easy to follow and ensure consistency throughout the code.

That's my opinion as well, and it also seems to be what we've been doing for all new and modified code.

@jitseniesen
Copy link
Member

I'm in favor of using 4-space hanging indents everywhere. It's clear, easy to follow and ensure consistency throughout the code.

Me too, with the exception of situations like

if some_long_function_name(
        param1, param2, param3):
    do_something()
    do_something_else()

where a 4-space indent in the second line would be quite confusing IMO.

@CAM-Gerlach
Copy link
Member

with the exception of situations like

Yes, agreed; PEP 8 proposed a few different ways to handle if, etc. blocks like that, but I recalled you mentioning that before as your preferred strategy and its consistent with how def is handled, so I'd already included it on the coding style page since we'd discussed in previously when it came up on a PR a while back.

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.

@dalthviz, I think this can be refactored to simplify it quite a lot. Please see my comment below.

spyder/app/mainwindow.py Outdated Show resolved Hide resolved
@ccordoba12 ccordoba12 changed the title PR: dark icons for regex, case sensitive and message box information PR: Make the internal console use the same theme as the other widgets Nov 22, 2018
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.

@dalthviz, I left my last round of comments. After that, this should be ready.

spyder/widgets/reporterror.py Show resolved Hide resolved
requirements/requirements.txt Show resolved Hide resolved
spyder/app/mainwindow.py Outdated Show resolved Hide resolved
spyder/app/mainwindow.py Outdated Show resolved Hide resolved
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 @dalthviz for your work on this one!

@ccordoba12 ccordoba12 merged commit 4445e32 into spyder-ide:master Nov 22, 2018
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.

Make internal console respect user's syntax highlighting theme
7 participants