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

Perforce completions #3314

Merged
merged 9 commits into from Aug 27, 2016

Conversation

Projects
None yet
4 participants
@nomaed
Contributor

nomaed commented Aug 19, 2016

p4 CLI tool completions.
There are completions for all of the top level commands, but only part of the sub-commands are currently implemented, so the support is not complete yet.
Additional sub-commands completions will be pushed by batches.

Show outdated Hide outdated share/completions/p4.fish
if test -e "$p4info"
return
end
if string match -qr '^Client unknown'

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

Missing "$p4info".

@faho

faho Aug 19, 2016

Member

Missing "$p4info".

Show outdated Hide outdated share/completions/p4.fish
function __fish_print_p4_client_name
set -l p4info (p4 info -s 2> /dev/null)
if test -e "$p4info"

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

Can this print a file/directory under certain circumstances? That's what test -e checks. Seems weird.

@faho

faho Aug 19, 2016

Member

Can this print a file/directory under certain circumstances? That's what test -e checks. Seems weird.

This comment has been minimized.

@nomaed

nomaed Aug 19, 2016

Contributor

Oh my bad, I used -e as in "empty", instead of -z

@nomaed

nomaed Aug 19, 2016

Contributor

Oh my bad, I used -e as in "empty", instead of -z

Show outdated Hide outdated share/completions/p4.fish
set -l result
for line in $changes
set line (string trim $line)
if test (string length $line) = 0

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

test -z "$line", or not string length -q $line.

@faho

faho Aug 19, 2016

Member

test -z "$line", or not string length -q $line.

Show outdated Hide outdated share/completions/p4.fish
set -l result
for line in $changes
set line (string trim $line)

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

Just do the string trim directly in the for loop - for line in (string trim -- $changes)

@faho

faho Aug 19, 2016

Member

Just do the string trim directly in the for loop - for line in (string trim -- $changes)

This comment has been minimized.

@nomaed

nomaed Aug 19, 2016

Contributor

I need to trim every line individually, since the output of p4 changes -L has empty lines, and others are indented.
Would using string trim operate on every line in the output, instead of just trimming the beginning end end of the entire buffer?

@nomaed

nomaed Aug 19, 2016

Contributor

I need to trim every line individually, since the output of p4 changes -L has empty lines, and others are indented.
Would using string trim operate on every line in the output, instead of just trimming the beginning end end of the entire buffer?

This comment has been minimized.

@floam

floam Aug 19, 2016

Member

Yeah:

floam@MacBook-Pro ~> set -l foo "   foo   "\n '    bar    '

floam@MacBook-Pro ~> echo $foo
   foo   
     bar    
floam@MacBook-Pro ~> string trim $foo
foo
bar
@floam

floam Aug 19, 2016

Member

Yeah:

floam@MacBook-Pro ~> set -l foo "   foo   "\n '    bar    '

floam@MacBook-Pro ~> echo $foo
   foo   
     bar    
floam@MacBook-Pro ~> string trim $foo
foo
bar
Show outdated Hide outdated share/completions/p4.fish
set result
end
set result $change_match[2]\t
if test "$detailed"

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

Please do an explicit test -n or string length -q.

This behavior of test is rather annoying.

@faho

faho Aug 19, 2016

Member

Please do an explicit test -n or string length -q.

This behavior of test is rather annoying.

This comment has been minimized.

@nomaed

nomaed Aug 19, 2016

Contributor

Saw this usage in another completion file, was surprised at first too.

@nomaed

nomaed Aug 19, 2016

Contributor

Saw this usage in another completion file, was surprised at first too.

Show outdated Hide outdated share/completions/p4.fish
if test (count $matches) -gt 1
echo -n \t$matches[2]
end
echo

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

Why this echo?

@faho

faho Aug 19, 2016

Member

Why this echo?

This comment has been minimized.

@nomaed

nomaed Aug 19, 2016

Contributor

Because the previous echos in the loop are echo -n that do not output a \n, and the function is supposed to print a new line for every iteration.

@nomaed

nomaed Aug 19, 2016

Contributor

Because the previous echos in the loop are echo -n that do not output a \n, and the function is supposed to print a new line for every iteration.

Show outdated Hide outdated share/completions/p4.fish
#########################################################
function __fish_print_p4_users
string replace -ar '(^\S+) <([^>]+)> \(([^\)]+)\).*$' '$1'\t'$3 <$2>' (p4 users)

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

These complicated regexes can always use an example as a comment (yes, I'm a hypocrite).

@faho

faho Aug 19, 2016

Member

These complicated regexes can always use an example as a comment (yes, I'm a hypocrite).

Show outdated Hide outdated share/completions/p4.fish
set -l base_types text binary symlink apple resource unicode utf16
set -l modifiers +m +w +x +k +ko +l +C +D +F +S +X
set -l mod_desc 'always set modtime' 'always writeable' 'exec bit set' '$Keyword$ expansion of Id, Header, Author, Date, DateUTC, DateTime, DateTimeUTC, DateTimeTZ, Change, File, Revision' '$Keyword$ expansion of Id, Header only' 'exclusive open: disallow multiple opens' 'server stores compressed file per revision' 'server stores deltas in RCS format' 'server stores full file per revision' 'server stores only single head rev., or specify number to <n> of revisions' 'server runs archive trigger to access files'
for i in $base_types

This comment has been minimized.

@faho

faho Aug 19, 2016

Member

This might be nicer as a multiline printf, e.g.

for i in base_types
    printf '%s\t%s\n' $i+m 'always set modtime' \
                            $i+w 'always writeable' \
# [...]

Which would also eliminate the math call (which forks off bc).

@faho

faho Aug 19, 2016

Member

This might be nicer as a multiline printf, e.g.

for i in base_types
    printf '%s\t%s\n' $i+m 'always set modtime' \
                            $i+w 'always writeable' \
# [...]

Which would also eliminate the math call (which forks off bc).

This comment has been minimized.

@nomaed

nomaed Aug 19, 2016

Contributor

I like it, definitely more readable.
I made the change.
However, fish_style joins it back to a single line :\

@nomaed

nomaed Aug 19, 2016

Contributor

I like it, definitely more readable.
I made the change.
However, fish_style joins it back to a single line :\

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 19, 2016

Member

This is already rather complicated (vcsen always are), so it's gonna take me a while to get through it.

Member

faho commented Aug 19, 2016

This is already rather complicated (vcsen always are), so it's gonna take me a while to get through it.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Aug 19, 2016

Member

This isn't as impossible to test as I feared - using the p4 binary without a license you can set P4PORT to public.perforce.com:1666 and play with it. Might help you test it out @faho if you're flying blind.

Member

floam commented Aug 19, 2016

This isn't as impossible to test as I feared - using the p4 binary without a license you can set P4PORT to public.perforce.com:1666 and play with it. Might help you test it out @faho if you're flying blind.

@nomaed

This comment has been minimized.

Show comment
Hide comment
@nomaed

nomaed Aug 19, 2016

Contributor

@floam I was note aware of the public server. This is great.
I was very frustrated by the need to VPN to our office network to use any command :)

Contributor

nomaed commented Aug 19, 2016

@floam I was note aware of the public server. This is great.
I was very frustrated by the need to VPN to our office network to use any command :)

@nomaed nomaed referenced this pull request Aug 19, 2016

Closed

Completions for P4 #3311

Show outdated Hide outdated share/completions/p4.fish
#########################################################
function __fish_p4_is_using_command
set -l cmd $argv[1]

This comment has been minimized.

@faho

faho Aug 22, 2016

Member

This function can be rewritten as contains -- "$argv[1]" (commandline -opc).

(Though what would be needed to properly model what p4 expects would probably be much more complicated, so keeping it as a function should be okay, so you can later expand it)

@faho

faho Aug 22, 2016

Member

This function can be rewritten as contains -- "$argv[1]" (commandline -opc).

(Though what would be needed to properly model what p4 expects would probably be much more complicated, so keeping it as a function should be okay, so you can later expand it)

Show outdated Hide outdated share/completions/p4.fish
#########################################################
# Negating __fish_p4_in_subcommand in complete -n doesn't seem to work,

This comment has been minimized.

@faho

faho Aug 22, 2016

Member

complete -c p4 -n 'not __fish_p4_in_subcommand' doesn't work? You probably forgot the quotes.

@faho

faho Aug 22, 2016

Member

complete -c p4 -n 'not __fish_p4_in_subcommand' doesn't work? You probably forgot the quotes.

This comment has been minimized.

@nomaed

nomaed Aug 22, 2016

Contributor

I have no idea why it didn't work, it works now when I tested it.
However, I also noticed that I am never using __fish_p4_in_subcommand, so I'll just remove it and use the "not" function only, even though aesthetically I'd rather have the positive named function 😉

@nomaed

nomaed Aug 22, 2016

Contributor

I have no idea why it didn't work, it works now when I tested it.
However, I also noticed that I am never using __fish_p4_in_subcommand, so I'll just remove it and use the "not" function only, even though aesthetically I'd rather have the positive named function 😉

Show outdated Hide outdated share/completions/p4.fish
function __fish_print_p4_client_name
set -l p4info (p4 info -s 2> /dev/null)
if test -z "$p4info"

This comment has been minimized.

@faho

faho Aug 22, 2016

Member

These test -zs aren't necessary because you're only doing string stuff later.

@faho

faho Aug 22, 2016

Member

These test -zs aren't necessary because you're only doing string stuff later.

This comment has been minimized.

@nomaed

nomaed Aug 22, 2016

Contributor

True, to avoid errors. However, there is a string match and a string match | string replace later on that will be executed. The test eliminates these 3 commands in case there's no p4 info result. That's my consideration.

Although I guess that the typical use case for this completion would be some user that has p4 set up and is in a p4 workspace. Otherwise the completions won't even be used anyhow. So it's probably safe to assume that this test will not be a benefit in real use case.

Can you estimate whether having this test would be non-negligible performance-wise?

@nomaed

nomaed Aug 22, 2016

Contributor

True, to avoid errors. However, there is a string match and a string match | string replace later on that will be executed. The test eliminates these 3 commands in case there's no p4 info result. That's my consideration.

Although I guess that the typical use case for this completion would be some user that has p4 set up and is in a p4 workspace. Otherwise the completions won't even be used anyhow. So it's probably safe to assume that this test will not be a benefit in real use case.

Can you estimate whether having this test would be non-negligible performance-wise?

This comment has been minimized.

@faho

faho Aug 22, 2016

Member

One of the benefits of string is that it's a builtin, so there's basically no startup time. That makes it great for operations on very few or very small strings (for bigger things, grep and sed et al are still faster).

A string on nothing should be quite fast. In fact it seems to be less than a millisecond (execute string match SOMETHING; echo $CMD_DURATION).

Usually, you don't need to care about the time string or test need, because usually the startup time of external tools is far greater. E.g. here, I'd estimate the p4 call to be by far the dominating call.

@faho

faho Aug 22, 2016

Member

One of the benefits of string is that it's a builtin, so there's basically no startup time. That makes it great for operations on very few or very small strings (for bigger things, grep and sed et al are still faster).

A string on nothing should be quite fast. In fact it seems to be less than a millisecond (execute string match SOMETHING; echo $CMD_DURATION).

Usually, you don't need to care about the time string or test need, because usually the startup time of external tools is far greater. E.g. here, I'd estimate the p4 call to be by far the dominating call.

Show outdated Hide outdated share/completions/p4.fish
# "Branch branch-name YYYY/MM/DD 'description text'"
set -l matches (string match -ar '^Branch\s+(\S+)[^\']+\'(.+)\'$' $branch)
set -l cnt (count $matches)
if test (count $matches) -lt 2

This comment has been minimized.

@faho

faho Aug 22, 2016

Member

I find if not set -q matches[2] to be a bit more idiomatic.

Similiarly the test (count below could be if set -q matches[2].

@faho

faho Aug 22, 2016

Member

I find if not set -q matches[2] to be a bit more idiomatic.

Similiarly the test (count below could be if set -q matches[2].

This comment has been minimized.

@nomaed

nomaed Aug 22, 2016

Contributor

It was supposed to be if test "$cnt" -lt 2, but your pattern looks clearer for fish, I'll use it.

@nomaed

nomaed Aug 22, 2016

Contributor

It was supposed to be if test "$cnt" -lt 2, but your pattern looks clearer for fish, I'll use it.

# "username <email@address> (Full Name) accessed YYYY/MM/DD"
# function will output it as:
# username[TAB]Full Name <email@address>
string replace -ar '(^\S+) <([^>]+)> \(([^\)]+)\).*$' '$1'\t'$3 <$2>' (p4 users)

This comment has been minimized.

@faho

faho Aug 22, 2016

Member

What happens if the email address is empty? (I'd suspect that'd be a very unusual edgecase, if it's even possible)

@faho

faho Aug 22, 2016

Member

What happens if the email address is empty? (I'd suspect that'd be a very unusual edgecase, if it's even possible)

This comment has been minimized.

@nomaed

nomaed Aug 22, 2016

Contributor

Tried removing the e-mail from my user, got:

Error in user specification.
Missing required field 'Email'.

So I guess it's a non issue really.

@nomaed

nomaed Aug 22, 2016

Contributor

Tried removing the e-mail from my user, got:

Error in user specification.
Missing required field 'Email'.

So I guess it's a non issue really.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 22, 2016

Member

Okay, a few minor nits, otherwise it's looking good.

Now I'll install p4 and actually try to use this.

Member

faho commented Aug 22, 2016

Okay, a few minor nits, otherwise it's looking good.

Now I'll install p4 and actually try to use this.

Fixed per review, added -d for all functions
Fixed per comments in review by @faho,
Added -d for all functions,
Renamed ”subcommand" term to “command” (so there’s probably diff noise)

@faho faho added this to the next-2.x milestone Aug 23, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 23, 2016

Member

@nomaed: I'll merge this weekend, so if we make a release on the 30th it's in. If you wish to add to it, I'll continue reviewing, though you can always submit new PRs.

Member

faho commented Aug 23, 2016

@nomaed: I'll merge this weekend, so if we make a release on the 30th it's in. If you wish to add to it, I'll continue reviewing, though you can always submit new PRs.

@nomaed

This comment has been minimized.

Show comment
Hide comment
@nomaed

nomaed Aug 23, 2016

Contributor

Great. I'll prepare a batch and will put it as a new PR, after the merge

Contributor

nomaed commented Aug 23, 2016

Great. I'll prepare a batch and will put it as a new PR, after the merge

@faho faho merged commit 5328d6b into fish-shell:master Aug 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Aug 27, 2016

Member

Merged, thanks!

Member

faho commented Aug 27, 2016

Merged, thanks!

@nomaed nomaed deleted the nomaed:p4-completions branch Aug 27, 2016

@krader1961 krader1961 modified the milestones: fish 2.4.0, next-2.x Sep 3, 2016

nomaed added a commit to nomaed/fish-shell that referenced this pull request Jul 17, 2017

Perforce completions (#3314)
* completions/p4.fish

* Updated per comments + added p4 clients

* p4 completions: integ, opened, reopen. "default" CL support.

* Perforce RCS -> SCM

* p4 reopen: list opened files

* Fixed per review, added -d for all functions

Fixed per comments in review by @faho,
Added -d for all functions,
Renamed ”subcommand" term to “command” (so there’s probably diff noise)

* p4 completions with submit list of files

* p4 completions for submit: lists open files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment