Extend browse command to open current directory or specific path in repo #437

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@mauricerkelly

I've added this little extension to the hub browse command that allows the user to browse to a specific file (or directory) on the source tree on github.com. I'm not a regular Ruby dev so I'm happily submitting this pull request as a means of obtaining feedback - I'm fully expecting to have failed to follow conventions or best practices somewhere in here and am happy to be pointed straight.

hub browse github-services -f
> open https://github.com/YOUR_LOGIN/github-services/blob/master

hub browse github-services -f lib/github-services.rb
> open https://github.com/YOUR_LOGIN/github-services/blob/master/lib/github-services.rb

I chose to use a -f flag so as not to conflict with the normal default action of browse. I realise that technically what it does can be replicated by calling something like the following line, but this option I've added doesn't require manually typing blob/BRANCH_NAME.

hub browse github-services blob/master/lib/github-services.rb
> open https://github.com/YOUR_LOGIN/github-services/blob/master/lib/github-services.rb

Additionally, if you are within your repo hierarchy the -f option will use relative paths:

cd lib
hub browse github-services -f github-services.rb
> open https://github.com/YOUR_LOGIN/github-services/blob/master/lib/github-services.rb
@mislav
Member
mislav commented Nov 22, 2013

Great minds think alike, it seems? :) #436

@mauricerkelly

Eeeek! I also see that failed the CI build. I'll go back and look at that - also, is it a feature that you would want included? Is there something that could be extracted from #436 or should an update be made to that pull request?

@mislav
Member
mislav commented Nov 22, 2013

Don't worry about the build failure, it's unrelated and likely my fault.

This PR is good (good job for a non-regular Ruby dev!) but I'm not sure whether I want this feature, or do I want to keep overloading the browse command. It's already doing way too much and its command-line API is kind of awkward.

The feature you've added for supporting relative paths is pretty cool, but could probably be done simpler with File.expand_path, which expands a relative path with regard to the current working directory. You would get an absolute filesystem path from which you'd just to have to strip away the project's root and you'd be set.

However there's an even easier way, courtesy of git ls-files:

$ cd lib
$ git ls-files --full-name hub.rb
lib/hub.rb

This also verifies that the file is actually present in version control.

@mauricerkelly

Thanks for the pointers. As with any language it seems it's all about learning the frameworks.

Would it be more useful if just called as follows:

hub browse -- <relative path>

If the supplied path does not exist it could fall back to the existing functionality (else "/#{subpage}"), but if it can be mapped to a real file in the repo (either relative to $PWD or to project root) it could browse directly to that file? I realise that behaviour is still technically overloading browse, but would get rid of the additional -f flag I proposed which might soften the impact a little? I'll make the changes for my own fork (the beauty of GitHub :-) but I'll update this PR in case you feel inclined to re-review.

Cheers again for taking the time to respond with advice.

Cheers,

Maurice

@mislav
Member
mislav commented Nov 22, 2013

You can keep pushing to the browse-files branch in your repo and the PR will get updated. Even if I decide against this, you are free to use that branch on your own machine.

The problem with hub browse -- <relative path> is that the 2nd argument to browse already is one of: tree wiki commits issues, and you would like to add the possibility that the 2nd argument might also be a file. This could lead to an edge case where there might be a tree or issues file in the current directory.

Not sure. I think I'd want the feature that you're proposing, but as part of an effort to split the browse command into several smaller commands to cut down on the overloading. I'll let you know when I have more ideas about this.

BTW, try adding tests in features/ instead of test/. It's where I write most of the tests now.

@mislav
Member
mislav commented Oct 21, 2014

Closing this PR as it won't apply anymore since we nuked the Ruby implementation and replaced it with Go. #642

It's on our roadmap to improve the browse command with said features. Stay tuned

@mislav mislav closed this Oct 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment