-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 switcher integration to projects #20895
PR: Add switcher integration to projects #20895
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @angelasofiaremolinagutierrez for all your work here! Left a couple of suggestions for some docstrings and some comments about the signal to open the switcher selection in the Editor. Also, left some comments/ideas regarding the usage of the switcher reference from the main widget. However, regarding that last part (usage of the switcher reference from the main widget), what do you think @ccordoba12 ?
I'm missing running this changes locally but seems like everything should be working 👍
If you have any comment or question regarding the comments in the review let me know!
- Add parameter with switcher items list. - Document fzf function
Co-authored-by: Daniel Althviz Moré <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked locally the PR and seems like some logic to handle the case when fzf
is not installed needs to be added? I tried without installing fzf
first and I got:
Traceback (most recent call last):
File "e:\acer\documentos\spyder\spyder otros\angela\spyder\spyder\plugins\projects\plugin.py", line 447, in handle_switcher_modes
self.get_widget().handle_switcher_modes("")
File "e:\acer\documentos\spyder\spyder otros\angela\spyder\spyder\plugins\projects\widgets\main_widget.py", line 617, in handle_switcher_modes
paths = self._execute_fzf_subprocess()
File "e:\acer\documentos\spyder\spyder otros\angela\spyder\spyder\plugins\projects\widgets\main_widget.py", line 732, in _execute_fzf_subprocess
out = subprocess.check_output(cmd_list, cwd=project_path,
File "C:\Users\dalth\anaconda3\envs\spyder-dev\lib\subprocess.py", line 424, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
File "C:\Users\dalth\anaconda3\envs\spyder-dev\lib\subprocess.py", line 505, in run
with Popen(*popenargs, **kwargs) as process:
File "C:\Users\dalth\anaconda3\envs\spyder-dev\lib\subprocess.py", line 951, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "C:\Users\dalth\anaconda3\envs\spyder-dev\lib\subprocess.py", line 1420, in _execute_child
hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
FileNotFoundError: [WinError 2] El sistema no puede encontrar el archivo especificado
Left new suggestions that maybe could help with the tests failing and the handling of the case when fzf
is not installed.
Testing locally, after installing fzf
and applying the suggested changes, things are looking good (a little bit of lag but that is expected 👍):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @angelasofiaremolinagutierrez for your work on this!
Co-authored-by: Carlos Cordoba <[email protected]> Co-authored-by: Daniel Althviz Moré <[email protected]>
- Delete unused functions - Delete debug prints - Add docstring to functions in widget - Move methods to private API
Since the switcher now waits for the user to stop typing, the waiting time needs to be greater.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last stylistic suggestions @angelasofiaremolinagutierrez, then this should be ready on my side.
Co-authored-by: Carlos Cordoba <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@angelasofiaremolinagutierrez, thanks for your hard work to implement this feature!! It's a great improvement!
Description of Changes
Future work:
Issue(s) Resolved
Fixes #3860
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:
@angelasofiaremolinagutierrez