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

Help string support for fish #264

Merged
merged 2 commits into from
Jul 8, 2020
Merged

Conversation

volkov
Copy link
Contributor

@volkov volkov commented Aug 8, 2019

Help string support for fish.

Work in progress.

dfs and _ARGCOMPLETE_DFS should probably be renamed to something more meaningful.

2019-08-08-162057_575x69_scrot

@codecov-io
Copy link

codecov-io commented Aug 8, 2019

Codecov Report

Merging #264 into master will increase coverage by 0.23%.
The diff coverage is 94.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   82.72%   82.95%   +0.23%     
==========================================
  Files           7        7              
  Lines         741      751      +10     
==========================================
+ Hits          613      623      +10     
  Misses        128      128              
Impacted Files Coverage Δ
argcomplete/shell_integration.py 100.00% <ø> (ø)
argcomplete/__init__.py 91.55% <94.44%> (+0.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 651e9fe...67e617f. Read the comment docs.

@volkov volkov force-pushed the feature/fishdocsupport branch 4 times, most recently from d05b7a7 to 02fa48e Compare August 8, 2019 21:29
argcomplete/__init__.py Outdated Show resolved Hide resolved
@volkov volkov changed the title WIP: Help string support for fish Help string support for fish Aug 9, 2019
@kislyuk
Copy link
Owner

kislyuk commented Aug 18, 2019

@volkov thanks for your work on this, and sorry about the delay in my review.

This looks good overall. Can you add a description of this feature, and a rendering of what it looks like in the shell, in the project README under the "fish support" section?

Also, it's certainly possible that completions will contain spaces (I'm sure you realize that completions can be produced for values, not just option names). In those cases, can you describe and paste renderings of what happens? How does this code interact with completions that contain both quotes and spaces? Since the only completions that contain spaces are value completions, and those are unlikely to have help, perhaps it's better to skip them instead of splitting them the way you are doing?

@volkov
Copy link
Contributor Author

volkov commented Aug 26, 2019

@kislyuk thanks for your review

Can you add a description of this feature, and a rendering of what it looks like in the shell, in the project README under the "fish support" section?

Yes, before finishing work on it.

Also, it's certainly possible that completions will contain spaces (I'm sure you realize that completions can be produced for values, not just option names). In those cases, can you describe and paste renderings of what happens? How does this code interact with completions that contain both quotes and spaces? Since the only completions that contain spaces are value completions, and those are unlikely to have help, perhaps it's better to skip them instead of splitting them the way you are doing?

I can't find how to set help string for value completions. Tried to use dict as value for choices, but it does not help. There is no clue in documentation also. Do you know how to set it?

I have added space to subparser name:

subparsers.add_parser('basic with space', help="basic help").add_argument('arg', choices=['foo', 'bar', 'baz'])

In this it drops help message:
2019-08-26-072626_569x73_scrot
Same with quotes:
2019-08-26-073431_1524x109_scrot

I found another issue - help message could contain _ARGCOMPLETE_DFS or _ARGCOMPLETE_IFS - in this case additional completions are added.

@volkov volkov force-pushed the feature/fishdocsupport branch 7 times, most recently from 782afd9 to 178acc8 Compare January 12, 2020 19:54
@volkov
Copy link
Contributor Author

volkov commented Jan 14, 2020

So I fixed problem with ifs in descriptions and added some documentation.

But I still don't know good solution space handling.

If I don't split I miss description for options with aliases, e.g help here:
2020-01-14-232143_1158x158_scrot

If I split (this is currently realized in PR) I miss descriptions for subcommands with spaces
2020-01-14-232024_1415x153_scrot

@evanunderscore
Copy link
Collaborator

From my reading, it looks like the original is trying to display all of the aliases together, e.g. -h --help -- show this help and exit, whereas you're trying to duplicate the help message for each parameter, e.g. -h (show this help message and exit) and --help (show this help message and exit).

Since the previous API hides the internal dict behind a function call get_display_completions, nothing should stop you from modifying what's stored in self._display_completions and then adding whatever logic you need in that function to restore it to what it was previously.

For example (not tested):

    def _get_option_completions(self, parser, cword_prefix):
        self._display_completions.update(
            # replace " ".join with tuple
            [[tuple(ensure_str(x) for x in action.option_strings
                       if ensure_str(x).startswith(cword_prefix)), action.help]
             for action in parser._actions
             if action.option_strings])
def get_display_completions():
    # " ".join the tuple to preserve previous API
    return {" ".join(k): v for k, v in self._display_completions}
        if dfs:
            display_completions = {key_part: value.replace(ifs, " ") if value else ""
                                   # use self._display_completions directly
                                   for key, value in self._display_completions.items()
                                   # no need to split key since it is already a tuple
                                   for key_part in key}

The actual change will be a little more involved since you'll either have to find all the other places in the code adding string keys and turn them into one element tuples, or account for both strings and tuples when you use the dict.

@volkov
Copy link
Contributor Author

volkov commented Feb 17, 2020

@evanunderscore that's good idea, I had changed pull request accordingly.

@volkov volkov force-pushed the feature/fishdocsupport branch 4 times, most recently from fa533b6 to 46f284c Compare February 17, 2020 20:22
@volkov
Copy link
Contributor Author

volkov commented Feb 17, 2020

And now I'm investigating problem with my code around argcomplete.CompletionFinder._get_subparser_completions

@volkov volkov changed the title Help string support for fish [wip] Help string support for fish Mar 28, 2020
@volkov volkov force-pushed the feature/fishdocsupport branch 3 times, most recently from 5cb820f to 67e617f Compare May 14, 2020 16:49
argcomplete/__init__.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #264 into master will increase coverage by 0.58%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   82.32%   82.91%   +0.58%     
==========================================
  Files           7        7              
  Lines         747      749       +2     
==========================================
+ Hits          615      621       +6     
+ Misses        132      128       -4     
Impacted Files Coverage Δ
argcomplete/shell_integration.py 100.00% <ø> (ø)
argcomplete/__init__.py 91.50% <93.75%> (+1.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d504584...bb14c9e. Read the comment docs.

@evanunderscore
Copy link
Collaborator

This looks great! Thanks for your persistence with this @volkov. I'm happy with the alias changes as illustrated by the changes to the test code, but since this is technically a breaking change I'll leave it to @kislyuk to sign off on it.

@volkov volkov changed the title [wip] Help string support for fish Help string support for fish Jul 4, 2020
@volkov
Copy link
Contributor Author

volkov commented Jul 4, 2020

@kislyuk have a look please

@kislyuk kislyuk merged commit 86fb4be into kislyuk:master Jul 8, 2020
@kislyuk
Copy link
Owner

kislyuk commented Jul 8, 2020

LGTM - thanks for sticking with this PR through the long development cycle @volkov. I'll roll a release shortly.

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.

5 participants