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: Use a menu separator constant instead of None #3795

Merged
merged 13 commits into from Dec 13, 2016

Conversation

mariacamilarg
Copy link
Contributor

Fixes #3794

@ccordoba12 ccordoba12 added this to the v3.1 milestone Dec 7, 2016
@@ -499,6 +499,10 @@ def show_std_icons():
dialog.show()
sys.exit(app.exec_())


def get_menu_separator():
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this menu_separator.

@@ -973,25 +973,27 @@ def get_plugin_actions(self):
# ---- File menu/toolbar construction ----
self.recent_file_menu = QMenu(_("Open &recent"), self)
self.recent_file_menu.aboutToShow.connect(self.update_recent_file_menu)


menu_separator = get_menu_separator()
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this assigment

@ccordoba12
Copy link
Member

Thanks for working on this @mariacamilaremolinagutierrez :-)

However, there are many other places where None is used as menu separator (i.e. not just in the Editor).

Please find them all and do the same replacement you already did for the Editor.

@ccordoba12
Copy link
Member

There are a lot of style failures. Please review CircleCI to see how to fix them :-)

@mariacamilarg
Copy link
Contributor Author

@ccordoba12 Wow there are, nice work on CI, it is super useful. I am making several commits but I'll squash the ones regarding style and some merges :)

Also, I'm not sure about this one:

E402: module level import not at top of file

Because that would mean changing the header structure:

image

Should I import it up?

@ccordoba12
Copy link
Member

Please don't change the import order in mainwindow. I carefully optimized it to start Spyder as fast as it's possible.

However, ciocheck is still reporting errors in plugins/editor.py and plugins/findinfiles.py

@mariacamilarg
Copy link
Contributor Author

@ccordoba12 do you know how to re run tests without making another commit? I'm getting a HTTPError: 500 Server Error in the CI failure.

@ccordoba12
Copy link
Member

I restarted the build. There are still some failures (besides the import order :-)

@mariacamilarg
Copy link
Contributor Author

@ccordoba12 I don't quite understand the last error I have left. How should I indent it?

image

image

@goanpeca
Copy link
Member

goanpeca commented Dec 10, 2016

@mariacamilaremolinagutierrez using \ to concatenate lines in Python is super ugly and I would strongly advice against using them. In this particular case you could concatenate stuff like this:

a = []
b = []
c = []
new = (a +
       b +
       ['something'] +
       c +
       ['something'])

You can also use the same for strings that are too long:

# a is a single string.
a = ('asdasdadsasdasd asd adasd ads ad asd asd ad ad ad asdas dasd asd adas '
     'asdasdasd asd s asd asd df a sd asd  d adsasdasd a ds asd  asd  asd  ad')

Parenthesis used like this serve the same purpose and make the code much cleaner.

Also PEP8 requires that the operators remain at the end of the line instead of at the beginning, so

# PEP8 ok
a = (5123123123123123123132134123123123132 +
     6)

# PEP8 fails
a = (5123123123123123123132134123123123132
     + 6)

Although that rule is not perfect to be honest...

style corrections

style corrections 2

style corrections 3

total style corrections
@mariacamilarg
Copy link
Contributor Author

@goanpeca Awesome! Thank you very much :)

@ccordoba12 The CI test that's failing is only the import one I mentioned earlier. And the style commits were squashed in one.

@@ -499,6 +499,10 @@ def show_std_icons():
dialog.show()
sys.exit(app.exec_())


def menu_separator():
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this one a constant instead of a function to use it more easily, like this

menu_separator = None

Copy link
Member

Choose a reason for hiding this comment

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

then make it upper case!

Copy link
Member

Choose a reason for hiding this comment

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

Ok, sure, no problem with that :-)

@ccordoba12
Copy link
Member

Great job at fixing the style issues!

I left a last comment, then this would be ready :-)

@rlaverde
Copy link
Member

rlaverde commented Dec 12, 2016

Also PEP8 requires that the operators remain at the end of the line instead of at the beginning

@goanpeca Both ways are accepted, and operators at the beginning Knuth's style are preferred
https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

@ccordoba12
Copy link
Member

Thanks @mariacamilaremolinagutierrez, this looks good to me now :-)

@ccordoba12 ccordoba12 changed the title PR: Use a menu_separator instead of None PR: Use a menu separator constant instead of None Dec 13, 2016
@ccordoba12 ccordoba12 merged commit d121b60 into spyder-ide:3.x Dec 13, 2016
ccordoba12 added a commit that referenced this pull request Dec 13, 2016
@mariacamilarg mariacamilarg deleted the menu_separator branch January 1, 2017 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants