4 ms·

Quickly review changed methods and functions in your pull requests

70 points by stablemap 9y ago

apeace 9y ago
I hope Github introduces this to the main code browser as well, not just PRs. I very often clone repos just so that I can open the code in an IDE, CTRL+Click a method and jump to its definition. It would be neat if Github had IDE-like features (but without becoming a platform for editing--just viewing!)
- alimoeeny 9y ago
  sourcegraph.com is your friend, at least for some languages.
tedmiston 9y ago
> Searching the file finder for a method or function in a Go, JavaScript, Ruby, or TypeScript file will provide you with a timeline-style view of the results, so you can skip to the most impactful parts of a pull request. It would be awesome to hear a timeline for rolling this out to more languages, especially Python.
nikic 9y ago
Tangentially related, my number one feature request for GitHub would be support for cross-referencing (a la Opengrok) in the code browser. Browsing through project code without a local checkout is one of the main things I do on GitHub, but it's not very ergonomic without cross-referencing. (I also realize that this is much to ask for, considering how much time Opengrok takes to index large codebases.)
- michaelmior 9y ago
  Sourcegraph[0] has a Chrome extension which provides a version of this feature. [0] https://sourcegraph.com https://sourcegraph.com
  - sqs 9y ago
    Sourcegraph founder here. The Chrome extension direct link is https://chrome.google.com/webstore/detail/sourcegraph-for-github/dgjhfomjieaadpoljlnidmbgkdffpack?hl=en https://chrome.google.com/webstore/detail/sourcegraph-for-gi.... Thanks for the mention!
zachrose 9y ago
This sounds like a step in a good direction. IMHO diff-based code review creates distorted incentives. Moving large pieces of code around feels harder to review, and adding an extra line or two feels easier to review. In the long term, this drives codebases in the wrong direction. (That 900-line method got there through hundreds of innocuous-looking PRs.) I hope eventually we get tools that make refactorings easy to review. I don't know what that would look like, or how well this can be done in today's languages, but there's room for improvement.
- sdesol 9y ago
  Having talked to both Microsoft and GitHub, I get the impression that intelligent code reviews, is an area of great interest for both companies. In the near future (~2 years), I can see us starting to review code in both the traditional way (line diffs) and in a more intelligent way (semantically and by code change impact). For example, if you moved a function 50 lines down and changed a string variable in that function, you'll be able to review and discuss the code changes like so: Semantic changes - Moved function FOO with public scope 50 lines down - Updated string variable FOO with public scope in function FOO Change impact - Moving function FOO 50 lines down does not change its scope. - Lines 56 and 30 in files X and Y in repositories Q and Z, reference the BAR variable. - Open pull request #35, contains FOO function changes as well. - Active releases 1.x and 2.x do not have these changes. The basic idea is, instead of only being able to review and comment on the lines changed, I fully expect us to be able to discuss their impact as well.
bcherny 9y ago
I wonder how the Souregraph guys feel about Github getting into their territory..
- sqs 9y ago
  Sourcegraph founder here. We love it. The more developers who are using code intelligence in their tools, the better the language support will be for all the various languages and repositories out there. That's good for developers everywhere, and we (Sourcegraph) could never build it all alone. Our master plan at https://sourcegraph.com/plan https://sourcegraph.com/plan describes what we're building on top of these basic "code intelligence" primitives, to help developers in all of their dev tools (not just GitHub), in all of their workflow, and in companies that have lots of code. And just like GitHub, we let people use these things for free on open-source so they can see how useful they are.
pmoriarty 9y ago
Can magit do this for code outside of github?
- cosmicexplorer 9y ago
  Actually not sure how to do this with magit, but I just tried `vc-region-history' while highlighting an R method signature and body and got a pretty slick view of all the relevant commits. Would definitely prefer magit for jumping to commits, might take a few lines of elisp. EDIT: Actually, [0] mentions =L, which seems to work flawlessly. Hacking on an option to take the selected region instead of manually specifying file and lines is easy. Alternatively, magit-blame lets you look at a specific change's commit. [0] https://github.com/magit/magit/issues/1717 https://github.com/magit/magit/issues/1717
petetnt 9y ago
Nice addition! If anyone at GitHub is reading this, the dropdown cannot be keyboard navigated because the dropdown doesn't scroll with the focus.

70 points by stablemap 9y ago

I hope Github introduces this to the main code browser as well, not just PRs. I very often clone repos just so that I can open the code in an IDE, CTRL+Click a method and jump to its definition. It would be neat if Github had IDE-like features (but without becoming a platform for editing--just viewing!)

alimoeeny 9y ago
sourcegraph.com is your friend, at least for some languages.

tedmiston 9y ago

> Searching the file finder for a method or function in a Go, JavaScript, Ruby, or TypeScript file will provide you with a timeline-style view of the results, so you can skip to the most impactful parts of a pull request. It would be awesome to hear a timeline for rolling this out to more languages, especially Python.

nikic 9y ago

Tangentially related, my number one feature request for GitHub would be support for cross-referencing (a la Opengrok) in the code browser. Browsing through project code without a local checkout is one of the main things I do on GitHub, but it's not very ergonomic without cross-referencing. (I also realize that this is much to ask for, considering how much time Opengrok takes to index large codebases.)

michaelmior 9y ago
Sourcegraph[0] has a Chrome extension which provides a version of this feature. [0] https://sourcegraph.com https://sourcegraph.com
- sqs 9y ago
  Sourcegraph founder here. The Chrome extension direct link is https://chrome.google.com/webstore/detail/sourcegraph-for-github/dgjhfomjieaadpoljlnidmbgkdffpack?hl=en https://chrome.google.com/webstore/detail/sourcegraph-for-gi.... Thanks for the mention!

zachrose 9y ago

This sounds like a step in a good direction. IMHO diff-based code review creates distorted incentives. Moving large pieces of code around feels harder to review, and adding an extra line or two feels easier to review. In the long term, this drives codebases in the wrong direction. (That 900-line method got there through hundreds of innocuous-looking PRs.) I hope eventually we get tools that make refactorings easy to review. I don't know what that would look like, or how well this can be done in today's languages, but there's room for improvement.

sdesol 9y ago
Having talked to both Microsoft and GitHub, I get the impression that intelligent code reviews, is an area of great interest for both companies. In the near future (~2 years), I can see us starting to review code in both the traditional way (line diffs) and in a more intelligent way (semantically and by code change impact). For example, if you moved a function 50 lines down and changed a string variable in that function, you'll be able to review and discuss the code changes like so: Semantic changes - Moved function FOO with public scope 50 lines down - Updated string variable FOO with public scope in function FOO Change impact - Moving function FOO 50 lines down does not change its scope. - Lines 56 and 30 in files X and Y in repositories Q and Z, reference the BAR variable. - Open pull request #35, contains FOO function changes as well. - Active releases 1.x and 2.x do not have these changes. The basic idea is, instead of only being able to review and comment on the lines changed, I fully expect us to be able to discuss their impact as well.

bcherny 9y ago

I wonder how the Souregraph guys feel about Github getting into their territory..

sqs 9y ago
Sourcegraph founder here. We love it. The more developers who are using code intelligence in their tools, the better the language support will be for all the various languages and repositories out there. That's good for developers everywhere, and we (Sourcegraph) could never build it all alone. Our master plan at https://sourcegraph.com/plan https://sourcegraph.com/plan describes what we're building on top of these basic "code intelligence" primitives, to help developers in all of their dev tools (not just GitHub), in all of their workflow, and in companies that have lots of code. And just like GitHub, we let people use these things for free on open-source so they can see how useful they are.

pmoriarty 9y ago

Can magit do this for code outside of github?

cosmicexplorer 9y ago
Actually not sure how to do this with magit, but I just tried `vc-region-history' while highlighting an R method signature and body and got a pretty slick view of all the relevant commits. Would definitely prefer magit for jumping to commits, might take a few lines of elisp. EDIT: Actually, [0] mentions =L, which seems to work flawlessly. Hacking on an option to take the selected region instead of manually specifying file and lines is easy. Alternatively, magit-blame lets you look at a specific change's commit. [0] https://github.com/magit/magit/issues/1717 https://github.com/magit/magit/issues/1717

petetnt 9y ago

Nice addition! If anyone at GitHub is reading this, the dropdown cannot be keyboard navigated because the dropdown doesn't scroll with the focus.