The Wayback Machine - https://web.archive.org/web/20210907043652/https://github.com/cli/cli/pull/4007
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

gh browse relative path enhancement #4007

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
Open

Conversation

@bchadwic
Copy link
Contributor

@bchadwic bchadwic commented Jul 16, 2021

Fixes #3952

gh browse enhancement that allows users to use relative paths when opening the browser.

assuming cli is the repository root

current functionality

C:\Users\User\Desktop\cli\pkg\cmd\browse> gh browse pkg\cmd\browse\browse.go
now opening https://github.com/cli/cli/tree/trunk/pkg/cmd/browse/browse.go in browser

new functionality

C:\Users\User\Desktop\cli\pkg\cmd\browse> gh browse
now opening https://github.com/cli/cli/ in browser

C:\Users\User\Desktop\cli\pkg\cmd\browse> gh browse pkg
now opening https://github.com/cli/cli/pkg in browser

C:\Users\User\Desktop\cli\pkg\cmd\browse> gh browse .
now opening https://github.com/cli/cli/tree/trunk/pkg/cmd/browse in browser

C:\Users\User\Desktop\cli\pkg\cmd\browse> gh browse .\browse.go
now opening https://github.com/cli/cli/tree/trunk/pkg/cmd/browse/browse.go in browser

C:\Users\User\Desktop\cli\pkg\cmd\browse> gh browse ..\..\..\api\cache.go:32
now opening https://github.com/cli/cli/tree/trunk/api/cache.go#L32 in browser

relative for linux

user@linux:~/Desktop/cli/pkg/cmd/browse$ gh browse ../../../api/cache.go:32
now opening https://github.com/cli/cli/tree/trunk/api/cache.go#L32 in browser

new functionality ((s) = operating system's path separator)
current folder = .
from current folder = .(s)
parent folder = ..(s)
absolute = folder | filename

@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Jul 16, 2021
@rsteube
Copy link
Contributor

@rsteube rsteube commented Jul 16, 2021

Don't think i would recommend supporting backslash as folder delimiter as this introduces inconsistencies between the shells.
It's also the escape character in many linux shells so that might get a bit confusing and problematic when people share their examples (already the case for special characters of course, but less likely to happen).

I would personally prefer the root folder to be the default is it is now and explicitly denote relative paths with ../ (parent folder) and ./ (current folder) prefixes.
When used outside of a repository this should cause an error.
When used with a different --repo it should assume that to be a fork (or have a similar folder structure) and thus try to open the relative path.

@vilmibm vilmibm self-requested a review Jul 16, 2021
@bchadwic
Copy link
Contributor Author

@bchadwic bchadwic commented Jul 19, 2021

Don't think i would recommend supporting backslash as folder delimiter as this introduces inconsistencies between the shells.
It's also the escape character in many linux shells so that might get a bit confusing and problematic when people share their examples (already the case for special characters of course, but less likely to happen).

Thanks for all of the feedback! I included both / and \ to make sure whatever is natural to the user / operating system is available. I take your point that examples might be confusing but ideally someone on linux should feel more inclined to use /, much like windows users would most likely use \. Could be wrong though

I would personally prefer the root folder to be the default is it is now and explicitly denote relative paths with ../ (parent folder) and ./ (current folder) prefixes.

Interesting, I also like this idea a lot but I would like another opinion. I'm not sure if this would make this feature too complex or not

When used outside of a repository this should cause an error.

gh browse should be taking care of this already, unless you mean starting in a repository, then using too many ../ until you are outside of the repo. In which case this will just open up to the main page of the repo

When used with a different --repo it should assume that to be a fork (or have a similar folder structure) and thus try to open the relative path.

I still need to test the --repo flag, but I see why you think it should be default "absolute" because of this. Assuming you are deep in one repository and want to look at another repository that isn't similar, this may cause issues.

@chemotaxis
Copy link
Contributor

@chemotaxis chemotaxis commented Jul 19, 2021

@vilmibm will have better ideas, but just wanted to mention that you should probably take a look at using path/filepath in the Go standard library to help handle cross-platform file path issues. The first time I used this package, it was kind of life changing.

There's this Stack Overflow answer too.

Copy link
Member

@vilmibm vilmibm left a comment

The design of this seems good to me, but as @chemotaxis mentioned this PR should be making use of the filepath Go package.

Definitely check out the file separator stuff since it'll give you cross platform support for free. Also consider making use of helpers like IsAbs.

@jcansdale
Copy link

@jcansdale jcansdale commented Jul 22, 2021

@bchadwic,

Note: gh browse still opens up to the root of the repository to avoid confusion, I would like input on this decision. If a user wants to open the folder they are in, in the browser, they can type out gh browse #

To open the folder I'm in, gh browse . would seem more intuitive. This would be the GitHub equivalent of open . on MacOS or start . on Windows.

bchadwic and others added 7 commits Jul 26, 2021
This reverts commit 5e3ca02.
…p)', parent folder '..(path sep)', absolute 'folder | filename'
current folder '.', from current folder '.(pathsep)', parent folder '..(path sep)', absolute 'folder | filename'
@bchadwic bchadwic requested a review from vilmibm Aug 4, 2021
@bchadwic
Copy link
Contributor Author

@bchadwic bchadwic commented Aug 4, 2021

Just bumping this, I revised the pr description at the top to show the current functionality. Looking forward to feedback on the new approach.

@mislav mislav requested review from cli/code-reviewers and removed request for cli/code-reviewers Aug 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
The GitHub CLI
  
Needs review 🤔
Linked issues

Successfully merging this pull request may close these issues.

5 participants