Skip to content

Improve autocompletion function on file name completion. #4842

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

Closed
wants to merge 33 commits into from

Conversation

kianasun
Copy link
Contributor

@kianasun kianasun commented Nov 5, 2017

No description provided.

@kianasun kianasun force-pushed the fix-completion branch 4 times, most recently from 397c4e3 to 0453412 Compare November 7, 2017 03:44
@pradyunsg
Copy link
Member

Hi @skx19952!

Thanks for this PR. Could you add a description to the PR to describe what issue this PR fixes?

Further, could you add some relevant content to the news file?

@pradyunsg pradyunsg added S: awaiting response Waiting for a response/more information type: enhancement Improvements to functionality labels Nov 7, 2017
@kianasun
Copy link
Contributor Author

kianasun commented Nov 7, 2017

Previous autocomplete function cannot autocomplete file names after options which have file, dir or path as metavar. For example, when typed pip install -r <Tab>, it should autocomplete all file names in current directory, or when typed pip --cache-dir <Tab>, it should autocomplete all directory names in current directory. Updated this function.

@pradyunsg pradyunsg removed the S: awaiting response Waiting for a response/more information label Nov 12, 2017
@pradyunsg
Copy link
Member

Aha! That's an awesome improvement!

I don't have the time right now to review this. I'll definitely come around to this soon (feel free to ping me if you want :P)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I like this change.

There's a comment about normalisation of paths, one comment about a 2 and a whole bunch of ✨ things I'd like to see. :)

(I'm a little picky when it comes to how things look. 🎨 ones -- you can skip if you want.)

:param opts: The available options to check
:return: path completion type(``file``, ``dir``, ``path`` or None)
"""
if cword >= 2 and cwords[cword - 2].startswith('-'):
Copy link
Member

Choose a reason for hiding this comment

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

A comment here explaining why 2 would be nice.

Copy link
Member

Choose a reason for hiding this comment

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

if cword < 2 or not cwords[cword - 2].startswith('-'):
    return
...

It would reduce one level of indentation.

:param completion_type: path completion type(`file`, `path` or `dir`)i
:return: A generator of regular files and/or directories
"""
# split ``current`` into two parts(directory and name)
Copy link
Member

Choose a reason for hiding this comment

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

In general, the single line comments that are present in this function aren't helpful.

(e.g. ``pip install -r``)
"""
res, env = setup_completion(
script, ('pip install -r r'),
Copy link
Member

@pradyunsg pradyunsg Nov 28, 2017

Choose a reason for hiding this comment

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

It would be nice if you could pass the arguments as keyword arguments -- it'll be easier to follow what's happening without having open up the definition of the function.

For all the following tests as well...

res, env = setup_completion(
script, ('pip install -r r'),
'3',
data.complete_paths
Copy link
Member

Choose a reason for hiding this comment

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

🎨 final comma

For all the following tests as well...

@@ -112,6 +112,10 @@ def indexes(self):
def reqfiles(self):
return self.root.join("reqfiles")

@property
def complete_paths(self):
return self.root.join("completepaths")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this folder and property to completion_paths?

@@ -113,14 +114,97 @@ def test_completion_for_default_parameters(script):

def test_completion_option_for_command(script):
"""
Test getting completion for ``--`` in command (eg. pip search --)
Test getting completion for ``--`` in command (e.g. ``pip search --``)
Copy link
Member

Choose a reason for hiding this comment

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

This change is good but unrelated? But I mean, it doesn't add much noise so it's fine. :P

'3',
data.complete_paths
)
assert 'requirements.txt' in res.stdout,\
Copy link
Member

Choose a reason for hiding this comment

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

Using parenthesis around the string allow skipping the trailing \, which looks nicer. :)

assert 'requirements.txt' in res.stdout, (
    "autocomplete function could not complete <file>"
    "after options in command"
)

For all the following tests as well...

@@ -114,6 +114,14 @@ def autocomplete():
options = [(x, v) for (x, v) in options if x not in prev_opts]
# filter options by current input
options = [(k, v) for k, v in options if k.startswith(current)]
# get completion type given cwords and available subcommand options
completion_type = get_path_completion_type(
cwords, cword, subcommand.parser.option_list_all)
Copy link
Member

Choose a reason for hiding this comment

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

🎨 move the ) to the next line with 1 level less indent and add a trailing comma.

('pip install -e ' + os.path.join(data.complete_paths, 'R')),
'3'
)
assert os.path.join(data.complete_paths, 'README.txt') in res.stdout,\
Copy link
Member

@pradyunsg pradyunsg Nov 28, 2017

Choose a reason for hiding this comment

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

Also, assert requirements.txt shows up, when normalisation of case may allow?

'2',
data.complete_paths
)
assert 'requirements.txt' not in res.stdout,\
Copy link
Member

Choose a reason for hiding this comment

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

Also, assert that README.txt doesn't show up, when normalisation of case might allow?

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I've requested changes above -- specifically about normalisation of path and some cleanups.

@kianasun kianasun force-pushed the fix-completion branch 9 times, most recently from 4fc2297 to 3c0c0e0 Compare December 3, 2017 18:04
@kianasun
Copy link
Contributor Author

kianasun commented Dec 3, 2017

I have made some changes. :)

pradyunsg and others added 18 commits March 28, 2018 15:42
pip uninstall no longer aborts if a package is not installed; instead
it prints a warning that the package is not installed and it is
skipping the uninstallation of it for this reason.
- Wheel doesn't have a public API
- Reword warning about installing into a running interpreter
* Add a global option to disable colors

 This is a fix for issue pypa#2449

 All it does is simply add a global option --no-color

 Internally it switches ColorizedStreamHandler to StreamHandler if
 this flag is detected.

* Fix lint errors

 * not sure it makes the code more readable though ...

* Fix typo

* Choose logging class before assigning

 * As requested per review
 * Make the code shorter and easier to follow

* Be polite to followers, add commas

* Add functional test for the --no-color output

* Better detection of windows

* Fix fragile tests - can't trust script --quiet

 * The version found in Travis-CI does not respect this flag
   It added a prefix line and suffix line found if one does not
   add the flag --quiet (script from util-linux 2.26.2).

   As such the out  put is:

     Script started on Fri 27 Oct 2017 07:17:30 AM CEST
     \x1b[31mCannot uninstall requirement noSuchPackage, not installed\x1b[0m\n

     Script done on Fri 27 Oct 2017 07:17:31 AM CEST

  With this change, the test should pass, and is hopefully more stable.

* Simplify testing for color or no-color
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Mar 28, 2018
@kianasun kianasun closed this Mar 29, 2018
@kianasun kianasun deleted the fix-completion branch March 29, 2018 04:00
@pradyunsg
Copy link
Member

@kianasun Why close the PR?

@kianasun
Copy link
Contributor Author

I am sorry. I think I did some wrong operations.. I will reopen it soon...

@pradyunsg
Copy link
Member

pradyunsg commented Mar 29, 2018 via email

@kianasun
Copy link
Contributor Author

Sorry, I think I need to open a new pull request.. @pradyunsg

@pradyunsg
Copy link
Member

pradyunsg commented Mar 29, 2018 via email

@kianasun
Copy link
Contributor Author

Hi, the new pull request is #5125.

@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.