Add support for `~` and `..` during file completion by anmolbabu · Pull Request #76 · posener/complete

anmolbabu · 2018-11-18T17:17:42Z

This PR adds support for auto-completion of file paths starting ~ and .. which
translate to user home dir

Signed-off-by: anmolbabu anmolbudugutta@gmail.com

codecov · 2018-11-18T17:20:14Z

Codecov Report

Merging #76 into master will decrease coverage by 4.15%.
The diff coverage is 53.27%.

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   89.93%   85.77%   -4.16%     
==========================================
  Files          14       15       +1     
  Lines         864      963      +99     
==========================================
+ Hits          777      826      +49     
- Misses         77      118      +41     
- Partials       10       19       +9

Impacted Files	Coverage Δ
args.go	`100% <100%> (ø)`	⬆️
predict_files.go	`95.12% <100%> (ø)`	⬆️
test_utils.go	`39.47% <39.47%> (ø)`
utils.go	`76.19% <83.33%> (+6.19%)`	⬆️

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 3ef9b31...0c3b234. Read the comment docs.

anmolbabu · 2018-11-18T17:23:50Z

@posener Please review
We would like support for ~ and ../

utils.go

anmolbabu · 2018-11-19T10:28:31Z

@posener Please review

Cc: @kadel @metacosm @mik-dass
Ref: redhat-developer/odo#960

posener

Hi,
Thanks for the PR :-)
It looks to me like this change spread on too many places than it should have.
I think that adding the resolveHomeDir function to the fixPathForm will suffice - as it is no difference than absolute path completion.
Also, if you could please add tests.
Thanks!

posener · 2018-11-23T08:49:33Z

utils.go

Can you use https://github.com/mitchellh/go-homedir instead?
It looks like there is a bit more code there, so it might cover more cases for more OSs.

Sure @posener

posener · 2018-11-23T08:50:11Z

predict_files.go

Why is this code commented out?

I am not sure why this shouldn't be there?
Its totally legal in my opinion for linux paths to end with /..
But for some reason(I assume bcoz listfiles doesn't list . and ..), that this doesn't work here and so its better we know that a trailing or in b/w usage of /.. doesn't work
So, I'll undo the change here

We can probably revisit it after this PR 😉

posener · 2018-11-23T09:02:06Z

utils.go

+		return strings.Replace(path, usr.HomeDir, "~", 1)
+	}
+
+	if strings.Contains(last, "..") {


The ../ completion on my machine works fine.
I think that the problem is that you tried to complete after .. and not after ../.
If not, can you elaborate which terminal and OS are you using?

@posener It doesn't seem to work for me(without this PR) on

[redhat@localhost odo]$ echo $SHELL /bin/bash [redhat@localhost odo]$ bash --version GNU bash, version 4.4.12(1)-release (x86_64-redhat-linux-gnu) Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software; you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. [redhat@localhost odo]$ cat /etc/*-release Fedora release 27 (Twenty Seven) NAME=Fedora VERSION="27 (Workstation Edition)" ID=fedora VERSION_ID=27 PRETTY_NAME="Fedora 27 (Workstation Edition)" ANSI_COLOR="0;34" CPE_NAME="cpe:/o:fedoraproject:fedora:27" HOME_URL="https://fedoraproject.org/" SUPPORT_URL="https://fedoraproject.org/wiki/Communicating_and_getting_help" BUG_REPORT_URL="https://bugzilla.redhat.com/" REDHAT_BUGZILLA_PRODUCT="Fedora" REDHAT_BUGZILLA_PRODUCT_VERSION=27 REDHAT_SUPPORT_PRODUCT="Fedora" REDHAT_SUPPORT_PRODUCT_VERSION=27 PRIVACY_POLICY_URL="https://fedoraproject.org/wiki/Legal:PrivacyPolicy" VARIANT="Workstation Edition" VARIANT_ID=workstation Fedora release 27 (Twenty Seven) Fedora release 27 (Twenty Seven)

Not even after ../

anmolbabu · 2018-11-25T12:34:52Z

Hi,

Hi @posener

Thanks for the PR :-)

My pleasure :)

It looks to me like this change spread on too many places than it should have.
I think that adding the resolveHomeDir function to the fixPathForm will suffice - as it is no difference than absolute path completion.

But the problem with ~ is that, the golang Apis(like filepath#Glob and filepath#Dir) do not resolve ~ properly(unlike .. for which the commit(2nd commit) seems small and honestly only specific to fixPathForm), however only shell does, so, while suggesting auto-complete, if my understanding is correct, we need to convert ~ expansion back to ~ at the time of suggesting auto-completion for consistency, and internally in code, resolve ~ to its expanded path which required changes to so many places.. I am now trying however, to see if they can be minimalist

Also, if you could please add tests.

Sure...

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

utils.go

anmolbabu · 2018-11-26T18:26:56Z

Add tests

@posener Can you take a brief look if the new form of the PR looks good
The PR now although localizes handling of ~ symbol to utils#fixPathForm, doesn't still avoid changes to other places but, the only difference is that now more places use fixPathForm ...
But kindly let me know if this can be further improved and I would happy to do the suggested changes

Note: The size of the PR has grown due to the committed vendor pkg(practice from my current project), For ease of review, I have broken the PR into multiple commits with the vendor going in a separate exclusive commit

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

predict_test.go

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu · 2019-01-30T11:25:40Z

@posener Ping
Gentle review reminder!!

posener · 2019-02-08T10:25:21Z

I'm sorry, but this PR became too big.

Please remove the glide files.
Maybe split to home dir completion and dot-dot completion? On my machine the dot-dot completion works, so I still don't understand what is wrong.

anmolbabu · 2019-02-11T07:49:18Z

I'm sorry, but this PR became too big.

Please remove the glide files.

I am using glide to pull over the github.com/mitchellh/go-homedir as a dependency to expand ~ to home dir
So, you meant to remove vendor/ right?
Did it in #79

Maybe split to home dir completion and dot-dot completion?

Sure..
Done as in #79 and #80

On my machine the dot-dot completion works, so I still don't understand what is wrong.

Quite some time now :) will revisit this again and check once but I see results of my previous experiments in #76 (comment) but will give this a retry...

And now since I have broken the PR into 2 I think atleast #79 is unblocked for review

@posener

anmolbabu · 2019-02-11T07:50:05Z

Abandoning this PR in favour of #79 and #80

golangcibot reviewed Nov 18, 2018

View reviewed changes

utils.go Outdated Show resolved Hide resolved

anmolbabu force-pushed the handle_tilde_previous_dir_symbols branch 2 times, most recently from 39758bc to 3bd6a30 Compare November 19, 2018 08:22

anmolbabu mentioned this pull request Nov 19, 2018

New suggestion system broke path suggestion in bash/zsh redhat-developer/odo#960

Closed

anmolbabu force-pushed the handle_tilde_previous_dir_symbols branch from 3bd6a30 to 7917859 Compare November 19, 2018 10:25

anmolbabu changed the title ~~Add support for ~ during file completion~~ Add support for ~ and .. during file completion Nov 19, 2018

posener reviewed Nov 23, 2018

View reviewed changes

anmolbabu added 2 commits November 25, 2018 18:21

Add support for ../

44cd99a

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Add support for ~ in auto completion

d62ef0b

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the handle_tilde_previous_dir_symbols branch from 7917859 to f2225fa Compare November 26, 2018 18:21

golangcibot reviewed Nov 26, 2018

View reviewed changes

utils.go Outdated Show resolved Hide resolved

anmolbabu added 3 commits November 29, 2018 17:05

Update glide files

6d7a0c9

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Add vendored pkgs

fbe1cee

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Fix golangci bot suggestions

5eb2df8

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the handle_tilde_previous_dir_symbols branch 2 times, most recently from a6eeb79 to 290ce91 Compare November 29, 2018 14:13

golangcibot reviewed Nov 29, 2018

View reviewed changes

predict_test.go Outdated Show resolved Hide resolved

Add test for ~ autocompletion

0c3b234

Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu force-pushed the handle_tilde_previous_dir_symbols branch from 290ce91 to 0c3b234 Compare November 29, 2018 15:02

anmolbabu closed this Feb 11, 2019

Conversation

anmolbabu commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anmolbabu commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

anmolbabu commented Nov 19, 2018

Uh oh!

posener left a comment

Choose a reason for hiding this comment

Uh oh!

posener Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

anmolbabu Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

posener Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

anmolbabu Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

anmolbabu Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

posener Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

anmolbabu Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolbabu Nov 23, 2018

Choose a reason for hiding this comment

Uh oh!

anmolbabu commented Nov 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

anmolbabu commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

anmolbabu commented Jan 30, 2019

Uh oh!

posener commented Feb 8, 2019

Uh oh!

anmolbabu commented Feb 11, 2019

Uh oh!

anmolbabu commented Feb 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

anmolbabu commented Nov 18, 2018 •

edited

Loading

codecov bot commented Nov 18, 2018 •

edited

Loading

anmolbabu commented Nov 18, 2018 •

edited

Loading

anmolbabu Nov 27, 2018 •

edited

Loading

anmolbabu Nov 23, 2018 •

edited

Loading

anmolbabu commented Nov 25, 2018 •

edited

Loading

anmolbabu commented Nov 26, 2018 •

edited

Loading