search

vermiculus / magithub

**DEPRECATED - please use Forge instead!** -- Magit-based interfaces to GitHub
GNU General Public License v3.0
579 stars 62 forks source link

Add new function magithub-browse-file #373

Closed manateelazycat closed 5 years ago

manateelazycat commented 5 years ago

Open git file in user browser

magithub-browse just open git repo magithub-browse-file is location file link in browser.

manateelazycat commented 5 years ago

@vermiculus Hi, your magithub is really cool.

I write a new function magithub-browse-file , please review code.

Thanks for magithub! ;)

vermiculus commented 5 years ago

Thanks! I fixed some issues with the test infrastructure that were preventing your PR from being tested. Can you rebase onto master?

vermiculus commented 5 years ago

This PR will implement requested feature #280.

manateelazycat commented 5 years ago

@vermiculus I have rebase new version, please review code, thanks! ;)

manateelazycat commented 5 years ago

@vermiculus I have use let-alist build new version, check again, thanks.

manateelazycat commented 5 years ago

@vermiculus Newest version with two changed:

1. Use if-let avoid call buffer-file-name twice
2. Use (magit-git-string "rev-parse" "--abbrev-ref" "HEAD") get current local branch

Leave message to me if you have any suggestion, I have to sleep, it's 24:00 in China. ;)

manateelazycat commented 5 years ago

@vermiculus Newest version is fine? Or any other improve suggestion?

vermiculus commented 5 years ago

I have not had the time to review. This could take up to a week or more. Keep in mind I have a full time job and a home life :-)

manateelazycat commented 5 years ago

I have not had the time to review. This could take up to a week or more. Keep in mind I have a full time job and a home life :-)

I also have full time job.

Unfortunately review one line code need a week or more.

No intention to offend, just feel that there is no contribution to enthusiasm, a little sad.

Anyway, hope you can review and merge my patch when you have time.

Have a nice day. ;)

manateelazycat commented 5 years ago

@vermiculus I have update new version, please review it when you have time.

Thanks

manateelazycat commented 5 years ago

@vermiculus

I upload new version that support argument file and line. Put everything in interactive is very ugly, I use &optional instead.

Please let me know if you have any suggestions.

Thanks!

-- Andy

vermiculus commented 5 years ago

Looks good. I'm curious though: why do you think it's ugly? It's idiomatic.

I've opened bbatsov/emacs-lisp-style-guide#51 for discussion and guidance.

manateelazycat commented 5 years ago

Looks good. I'm curious though: why do you think it's ugly? It's idiomatic.

I've opened bbatsov/emacs-lisp-style-guide#51 for discussion and guidance.

Haha, It's personal style.

I prefer use &optional style making the first impression of program feels very simple, then other people more willing try to contribute.

It will make other people feels complicated if we put everything in interactive expression.

BTW, it's time to merge ? haha.

vermiculus commented 5 years ago

I don't agree with your reasoning, but I'll agree that it's good enough to merge. I may change my mind about the interactive form later after it's merged, though :-)

Please squash your commits into one.

manateelazycat commented 5 years ago

I don't agree with your reasoning, but I'll agree that it's good enough to merge. I may change my mind about the interactive form later after it's merged, though :-)

Please squash your commits into one.

Hmm, can't you just click "Merged PR" button in github ? ;)

vermiculus commented 5 years ago

I still prefer to merge only one squashed commit to keep the history clean and understandable.

manateelazycat commented 5 years ago

@vermiculus Please review https://github.com/vermiculus/magithub/pull/377

I close this PR now, thanks!

vermiculus commented 5 years ago

In the future, force-push to the pull request branch instead. You do not need to create a new pull request.