Add support for `~` and `..` during file completion #76

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 [email protected]

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

@@ -37,6 +51,15 @@ func fixPathForm(last string, file string) string {
 	return fixDirPath(rel)
 }

+func resolveHomeSymbol(path string) string {


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

-	if strings.HasSuffix(a.Last, "/..") {
-		return nil
-	}
+	/*


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 <[email protected]>

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 <[email protected]>

predict_test.go

Signed-off-by: anmolbabu <[email protected]>

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 <[email protected]>

Add support for ~ in auto completion

d62ef0b

Signed-off-by: anmolbabu <[email protected]>

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 <[email protected]>

Add vendored pkgs

fbe1cee

Signed-off-by: anmolbabu <[email protected]>

Fix golangci bot suggestions

5eb2df8

Signed-off-by: anmolbabu <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `~` and `..` during file completion #76

Add support for `~` and `..` during file completion #76

anmolbabu commented Nov 18, 2018 •

edited

Loading

codecov bot commented Nov 18, 2018 •

edited

Loading

anmolbabu commented Nov 18, 2018 •

edited

Loading

anmolbabu commented Nov 19, 2018

posener left a comment

posener Nov 23, 2018

anmolbabu Nov 23, 2018

posener Nov 23, 2018

anmolbabu Nov 26, 2018

anmolbabu Nov 27, 2018 •

edited

Loading

posener Nov 23, 2018

anmolbabu Nov 23, 2018 •

edited

Loading

anmolbabu Nov 23, 2018

anmolbabu commented Nov 25, 2018 •

edited

Loading

anmolbabu commented Nov 26, 2018 •

edited

Loading

anmolbabu commented Jan 30, 2019

posener commented Feb 8, 2019

anmolbabu commented Feb 11, 2019

anmolbabu commented Feb 11, 2019

Add support for ~ and .. during file completion #76

Add support for ~ and .. during file completion #76

Conversation

anmolbabu commented Nov 18, 2018 • edited Loading

codecov bot commented Nov 18, 2018 • edited Loading

Codecov Report

anmolbabu commented Nov 18, 2018 • edited Loading

anmolbabu commented Nov 19, 2018

posener left a comment

Choose a reason for hiding this comment

posener Nov 23, 2018

Choose a reason for hiding this comment

anmolbabu Nov 23, 2018

Choose a reason for hiding this comment

posener Nov 23, 2018

Choose a reason for hiding this comment

anmolbabu Nov 26, 2018

Choose a reason for hiding this comment

anmolbabu Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

posener Nov 23, 2018

Choose a reason for hiding this comment

anmolbabu Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

anmolbabu Nov 23, 2018

Choose a reason for hiding this comment

anmolbabu commented Nov 25, 2018 • edited Loading

anmolbabu commented Nov 26, 2018 • edited Loading

anmolbabu commented Jan 30, 2019

posener commented Feb 8, 2019

anmolbabu commented Feb 11, 2019

anmolbabu commented Feb 11, 2019

Add support for `~` and `..` during file completion #76

Add support for `~` and `..` during file completion #76

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