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 option to use Jedi in the IPython console + warning on greedy completer #6832

Merged
merged 6 commits into from Mar 27, 2018

Conversation

hofingermarkus
Copy link

@hofingermarkus hofingermarkus commented Mar 24, 2018

This adds an option for Jedi Code completion in the ipython shell.

It also adds a warning text for the Greedy completer, to inform users, that the current version of the Greedy completer in ipython needs an extra space to split commands.
Related Spyder-ide issues: #6803 #6700

Related Ipython Console Issues:
https://github.com/ipython/ipython/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+greedy+completer

spyder_gui_jedi_update3

max added 2 commits March 24, 2018 14:10
Also updated GUI text of Greedy completion to warn about greedy completion bug.
Also updated GUI text of Greedy completion to warn about greedy completion bug.
@ccordoba12 ccordoba12 added this to the v3.2.9 milestone Mar 24, 2018
@ccordoba12 ccordoba12 changed the title PR: Added GUI option to use Jedi code completion in the iypthon console + Warning on Greedy Completer Bug PR: Added option to use Jedi the IPython console + warning on greedy completer Mar 24, 2018
@CAM-Gerlach CAM-Gerlach changed the title PR: Added option to use Jedi the IPython console + warning on greedy completer PR: Add option to use Jedi the IPython console + warning on greedy completer Mar 24, 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.

Thanks for your contribution! I found a number of elementary typos and errors in the text; please correct them. Also, overall, the total amount of text really needs to be cut down, and there's quite a bit of crazy and inconsistent formatting; I've provided a few suggestions to try to address both. Finally, can you add a simple test for this?

@@ -477,15 +477,42 @@ def setup_page(self):
run_file_group.setLayout(run_file_layout)

# ---- Advanced settings ----
# Enable Jedi completion in Ipyhton Console
Copy link
Member

Choose a reason for hiding this comment

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

"IPython" is the correct spelling and capitalization, though ipython also works.

@@ -477,15 +477,42 @@ def setup_page(self):
run_file_group.setLayout(run_file_layout)

# ---- Advanced settings ----
# Enable Jedi completion in Ipyhton Console
ipy_jedi_group = QGroupBox(_("Jedi completion in Ipython console"))
Copy link
Member

Choose a reason for hiding this comment

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

IPython Console, to follow the above convention. Also, needs the article "the" in front of it. So:

"Jedi Completion in the IPython Console"

However, as this is a section title, for consistancy with the general style and with the others on the page, it should be shortened to:

"Jedi Completion"

or something similar, and in Title Case.

@@ -477,15 +477,42 @@ def setup_page(self):
run_file_group.setLayout(run_file_layout)

# ---- Advanced settings ----
# Enable Jedi completion in Ipyhton Console
ipy_jedi_group = QGroupBox(_("Jedi completion in Ipython console"))
ipy_jedi_label = QLabel(_("Enable Jedi based <tt>Tab</tt> completion in"
Copy link
Member

Choose a reason for hiding this comment

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

"Jedi-based", also needs a space at the end.

# Enable Jedi completion in Ipyhton Console
ipy_jedi_group = QGroupBox(_("Jedi completion in Ipython console"))
ipy_jedi_label = QLabel(_("Enable Jedi based <tt>Tab</tt> completion in"
"the Ipython console. E.g. completion of"
Copy link
Member

Choose a reason for hiding this comment

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

"the IPython Console; works with e.g. nested lists, etc.

Copy link
Member

Choose a reason for hiding this comment

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

Alternately, you can cut everything after the semicolon and instead add "similar to the greedy completer, but without evaluating the code." A little longer and more visually awkward than cutting the second line, but cleaner flow on the textual side.

ipy_jedi_group = QGroupBox(_("Jedi completion in Ipython console"))
ipy_jedi_label = QLabel(_("Enable Jedi based <tt>Tab</tt> completion in"
"the Ipython console. E.g. completion of"
"nested lits etc. This is similar to the "
Copy link
Member

Choose a reason for hiding this comment

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

I'd lean toward cutting this sentence, as it saves a full line, and including it in the popup tip instead; better IMO to put that extra descriptive text in the popup tip as its designed for then clutter the UI with every last detail.

# Greedy completer group
greedy_group = QGroupBox(_("Greedy completion"))
greedy_group = QGroupBox(_("Greedy completion in Ipython console"))
greedy_label = QLabel(_("Enable <tt>Tab</tt> completion on elements "
"of lists, results of function calls, etc, "
"<i>without</i> assigning them to a "
"variable.<br>"
"For example, you can get completions on "
Copy link
Member

Choose a reason for hiding this comment

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

Since you're adding so much extra text, I'd recommend you simplify this to something like:

"Get completions on e.g. [...] and [...]"

@@ -575,7 +602,7 @@ def setup_page(self):
_("Graphics"))
tabs.addTab(self.create_tab(run_lines_group, run_file_group),
_("Startup"))
tabs.addTab(self.create_tab(greedy_group, autocall_group, sympy_group,
tabs.addTab(self.create_tab(ipy_jedi_group, greedy_group , autocall_group, sympy_group,
Copy link
Member

Choose a reason for hiding this comment

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

Remove extra space added after greedy_group.

greedy_label = QLabel(_("Enable <tt>Tab</tt> completion on elements "
"of lists, results of function calls, etc, "
"<i>without</i> assigning them to a "
"variable.<br>"
"For example, you can get completions on "
"things like <tt>li[0].&lt;Tab&gt;</tt> or "
"<tt>ins.meth().&lt;Tab&gt;</tt>"))
"<tt>ins.meth().&lt;Tab&gt;</tt> <br><br>"
"<b>ATTENTION:<br> Ipython's Greedy completer has "
Copy link
Member

Choose a reason for hiding this comment

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

"IPython's greedy completer has "

Or better yet, rewrite the whole thing to read as follows:

"Due to a bug, IPython's greedy completer requires a leading space for some completions; e.g. np.sin(<Space>np.<Tab>) works, while np.sin(np.<Tab>) doesn't.

"<b>ATTENTION:<br> Ipython's Greedy completer has "
"a bug</b> that requires a leading "
"<tt>&lt;Space&gt;</tt> "
"for some completions to work. e.g. chane "
Copy link
Member

Choose a reason for hiding this comment

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

Typo; I presume you mean "change", and e.g. needs to be capitalized if starting a sentence, though I recommend rewriting it as above.

"for some completions to work. e.g. chane "
"<tt>np.sin(np.&lt;Tab&gt;</tt> "
"to <tt>np.sin(&lt;Space&gt;np.&lt;Tab&gt;</tt> "
", and completion for string filenames has "
Copy link
Member

Choose a reason for hiding this comment

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

Everything after the comma on this line is redundant, you used the same phrase above and it is directly implied by what you've stated. I know it relates to your specific problem, but including every possible case (and this is one few if any others have complained about) is an issue for the documentation IMO, not the UI.

…ompletion in the iypthon console.

Also updated GUI text of Greedy completion to warn about greedy completion bug.
@pep8speaks
Copy link

pep8speaks commented Mar 24, 2018

Hello @hofingermarkus! Thanks for updating the PR.

Line 483:35: E127 continuation line over-indented for visual indent
Line 489:74: W291 trailing whitespace
Line 490:30: E127 continuation line over-indented for visual indent
Line 495:1: W293 blank line contains whitespace
Line 500:1: W293 blank line contains whitespace
Line 604:80: E501 line too long (90 > 79 characters)

Comment last updated on March 27, 2018 at 07:51 Hours UTC

…ompletion in the iypthon console.

Also updated GUI text of Greedy completion to warn about greedy completion bug.
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.

Looking good. Now just fix the last few PEP8 issues, update the screenshot, and add a test for enabling/disabling and basic functionality of jedi completion (spyder/plugins/tests/test_ipythonconsole.py should give you some good examples of what to do) and you should be good, pending further @ccordoba12 feedback. Thanks!

@CAM-Gerlach CAM-Gerlach dismissed their stale review March 24, 2018 21:19

Changes executed as requested. Thanks!

@@ -158,6 +158,7 @@
'startup/use_run_file': False,
'startup/run_file': '',
'greedy_completer': False,
'ipy_jedi_completer': False,
Copy link
Member

Choose a reason for hiding this comment

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

Please change this to jedi_completer. We already know that it's an IPython console option because it's in the right section.

"when working with large dataframes!"))
ipy_jedi_label.setWordWrap(True)
ipy_jedi_box = newcb(_("Use Jedi completion in the IPython console"),
"ipy_jedi_completer",
Copy link
Member

Choose a reason for hiding this comment

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

Change to jedi_completer

@@ -477,20 +477,46 @@ def setup_page(self):
run_file_group.setLayout(run_file_layout)

# ---- Advanced settings ----
# Enable Jedi completion in IPyhton Console
ipy_jedi_group = QGroupBox(_("Jedi completion"))
Copy link
Member

Choose a reason for hiding this comment

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

Remove the ipy_ prefix from all variables in this section. Again, these are IPython confg options, so there's no need for that prefix.

@@ -124,6 +124,7 @@ def env(self):
'SPY_RUN_FILE_O': CONF.get('ipython_console', 'startup/run_file'),
'SPY_AUTOCALL_O': CONF.get('ipython_console', 'autocall'),
'SPY_GREEDY_O': CONF.get('ipython_console', 'greedy_completer'),
'SPY_IPY_JEDI_O': CONF.get('ipython_console', 'ipy_jedi_completer'),
Copy link
Member

Choose a reason for hiding this comment

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

Again, remove IPY_ and ipy_ here.

@@ -69,10 +69,12 @@ def kernel_config():
# Until we implement Issue 1052
spy_cfg.InteractiveShell.xmode = 'Plain'

# Jedi completer in IPpthon console
ipy_jedi_o = os.environ.get('SPY_IPY_JEDI_O') == 'True'
Copy link
Member

Choose a reason for hiding this comment

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

Remove ipy_ and IPY_ also here.

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.

This looks good, except for some minor formatting issues.

@@ -69,10 +69,12 @@ def kernel_config():
# Until we implement Issue 1052
spy_cfg.InteractiveShell.xmode = 'Plain'

# Jedi completer in IPpthon console
Copy link
Member

Choose a reason for hiding this comment

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

Remove here in IPpthon console.

Copy link
Member

Choose a reason for hiding this comment

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

This is still missing. Please address it.

Copy link
Author

Choose a reason for hiding this comment

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

done

@hofingermarkus
Copy link
Author

Updated according to change requests.
Added new updated Screenshot to PR

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 @hofingermarkus! Great work here!

@ccordoba12 ccordoba12 changed the title PR: Add option to use Jedi the IPython console + warning on greedy completer PR: Add option to use Jedi in the IPython console + warning on greedy completer Mar 27, 2018
@ccordoba12 ccordoba12 merged commit 66c094e into spyder-ide:3.x Mar 27, 2018
ccordoba12 added a commit that referenced this pull request Mar 27, 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.

None yet

4 participants