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 disambiguation for dedicated IPython consoles #4700

Merged
merged 5 commits into from Jul 6, 2017

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Jul 5, 2017

Fixes #4694

self.clients.insert(index_to, client)
self.update_plugin_title.emit()

def get_tab_text(self, fname):
Copy link
Member

Choose a reason for hiding this comment

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

get_tab_text -> disambiguate_fname

self.clients.insert(index_to, client)
self.update_plugin_title.emit()

def get_tab_text(self, fname):
"""Get tab text without ambiguation."""
Copy link
Member

@ccordoba12 ccordoba12 Jul 5, 2017

Choose a reason for hiding this comment

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

Let's change this to

"""Generate a file name without ambiguation."""


# Don't increase the count of master clients
self.master_clients -= 1

# Rename client tab with filename
client = self.get_current_client()
client.allow_rename = False
self.rename_client_tab(client, filename)
fname = self.get_tab_text(filename)
Copy link
Member

Choose a reason for hiding this comment

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

fname -> tab_text

"""Get tab text without ambiguation."""
files_path_list = [filename for filename in self.filenames
if filename is not None]
return sourcecode.get_file_title(files_path_list, fname)
Copy link
Member

Choose a reason for hiding this comment

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

Please rename get_file_title in sourcecode to disambiguate_fname and make the corresponding change in the Editor.

@ccordoba12
Copy link
Member

@dalthviz, I left several comments related to renaming variables and functions to better understand your code.

Also, this is missing a test. It could be like this:

  1. Add it to test_ipythonconsole.py
  2. You can create two clients with these filenames: /tmp/a/b/c.py and /tmp/a/d/e.py.
  3. You must assert that the tab names are b/c.py and d/e.py (if I understand our disambiguation algorithm correctly :-).

@ccordoba12
Copy link
Member

I think this is ready. @dalthviz, could you post a screenshot to see how this looks, please?

@@ -907,7 +909,7 @@ def write_to_stdin(self, line):

@Slot()
@Slot(bool)
Copy link
Member

Choose a reason for hiding this comment

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

Please add two more signatures below this line

@Slot(object)
@Slot(bool, object)

to cover all possible entries of create_new_client.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, give me a moment to think this.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be

@Slot(str)
@Slot(bool, str)

and you need to change below

filename=''

instead of

filename=None

@dalthviz
Copy link
Member Author

dalthviz commented Jul 5, 2017

Preview:

Running two files with paths like:
.../a/b/c.py
.../a/d/c.py

imagen

def disambiguate_fname(self, fname):
"""Generate a file name without ambiguation."""
files_path_list = [filename for filename in self.filenames
if filename is not None]
Copy link
Member

@ccordoba12 ccordoba12 Jul 5, 2017

Choose a reason for hiding this comment

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

With my last suggested change, you need to change this line to

if filename]

instead of

if filename is not None]

def update_tabs_text(self):
"""Update the text from the tabs."""
for index, fname in enumerate(self.filenames):
if fname is not None:
Copy link
Member

Choose a reason for hiding this comment

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

I think this also needs to change to

if fname:

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.

Great job @dalthviz!

@ccordoba12 ccordoba12 merged commit 9cb8924 into spyder-ide:3.x Jul 6, 2017
ccordoba12 added a commit that referenced this pull request Jul 6, 2017
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

2 participants