When looking through an long and tricky diff there can be many comments, most of which are done. Differential has the ability to hide individual comments but it annoying to hide all the done ones. Its a manual process and its easy to hide the non-done ones.
This is a feature request from multiple of our users (to try and give you some context about why I'm submitting it).
This is especially true for long-lived reviews. I've been implementing a 1-wire bus. I thought it was ready when I submitted it to phabricator, but a number of comments have surfaced (in my case about the man pages, but in other long-lived reviews it is about the code). As the code changes and evolves, there's a natural motion of the comments people leave form the actual code it affects. This is doubly true if the comments are addressed. Tripply true when the comments may be short, but suggest rather major surgery (eg, you should move from a polled model to an interrupt driven state machine).
In my case, I'd love to hide all the old comments (at least by default) so that new reviewers aren't distracted by them. I'm not trying to hide anything, but when reviewers can ignore the obsolete comments, they can use the time they spent ignoring them to review my code instead.
+1
It's very difficult to dive into the current state of play in a long lived diff as the code is split into pieces between each comment.
+1 as well. Long diff, lots of comments, comments no longer aligned to their code (because line numbers shift) = unreadable mess.
Here are the interactions I can come up with which we'll potentially get wrong by implementing this naively:
The similar issues in T8420 should also be resolved concurrently.
I generally don't like completely vanishing information with no UI cue, either. "Hide" should probably really mean "fold into the gutter" (as hide on individual comments does) and/or we should perhaps look at restoring the "buoyant" header and possibly putting state/controls there (T1591).
Some of this is almost certainly adjacent: T4043, T10049, T9270, T7838, T8288, T8047, T10668, T10954, T11093.
This tasks suffers from a lack of clear implementation boundaries and it could end up taking days to clear related work if it cascades into a majority of that list, but the value of the feature is presumably comparatively low given that this is likely "solvable" -- just not in a maintainable/support-friendly way -- with a small bookmarklet or browser extension.
I vaguely wonder if modularizing keyboard commands is worth considering. In theory, we could let you restore behaviors like /-to-focus-search and provide i-to-hackily-toggle-inline-modes through extensions. One issue is that it seems difficult for command extensions to select when they should activate, because neither URL-based or controller instanceof X-based checks seem particularly good. We'd probably have to have every modular keyboard behavior activate on every page and then test if whatever it's looking for is actually present in the DOM, which feels messy. And I don't really see a large set of use cases for this, and don't want to encourage development that treats the DOM as an API (see also T4213), and building this is not likely easier than implementing this task (hiding inlines) properly.
Anyway, I think the ways forward on this are basically:
I think the desired feature is reasonable, this class of interactions is just fragile already and I don't think all pathways that leave us better off than we started have a larger cost than the value of the feature.
(In some theoretical free market version of our scheduling process, the priority queue would be more of an auction and there would be a way to say "this is worth X, independent of the number of hours it takes", and we could then just divide that by implementation cost and work down the queue in value-order. I might still try to experiment with this at some point, but I think it's probably a poor match for the real world and a particularly poor match for enterprise billing departments.)
I'm moving this to "Rugged Terrain", as the best available pathway forward leaves us with dramatically higher implementation cost than the reasonable value of the feature. We can pursue this after tasks with better aligned costs and value are addressed.
Broadly, the technical stuff here is in fairly good shape now but I still don't truly understand what problem we're solving. From earlier, I dug this up:
I've been implementing a 1-wire bus.
I believe this is the corresponding change: https://reviews.freebsd.org/D2956
At time of writing, this change affected 2,493 lines of code, had received 101 inline comments over approximately 23 days (unclear what became of it).
Of the 101 inline comments, about 90 appear to be related to grammar or spelling. 2 were humans catching lint issues (tab, extra blank line). About 5 appear to be actual high-level human discussion of program behavior. The remainder are discussion and notes-to-self (I counted all this by hand, so these numbers may not be exactly accurate or add up).
Phabricator isn't very good software for processing 90 minor spelling and grammar corrections, but it isn't intended to be, and I'm not sure it should be.
As a reviewer, I probably would not leave these inlines. I would either give the author high-level feedback ("run this through a spellchecker -- for example, I caught at least 6 instances of 'resistor' being spelled wrong") or do the edits myself ("code looks good but the grammar in some of the docs is a bit rough, let's split that out into a separate change and I'll counter-diff you with some improvements?"). These approaches require less of my time -- and less of the author's time -- than leaving 90 separate inlines, 6 of which point out misspellings of "resistor".
I don't think any author at any skill level benefits from this sort of granular review, either: even students/interns would be better served by high-level feedback ("use a spellchecker") that points them toward tools they can use to improve.
I currently plan to implement this request literally, without an understanding of a specific problem which it represents a good solution to. I continue to basically believe that all cases where this is important are cases of review going off the rails and exploding in a fiery wreck, and that a much better intervention was available earlier in the pipeline (like: provide higher-level feedback, use lint, or find a better approach to the problem).
If you don't agree with this, it would be helpful if anyone can provide live examples of reviews with large amounts of meaningful discussion and feedback which these solutions would help with. I'm open to understanding this problem better, it just seems like every time I actually see one of these revisions it's someone leaving dozens of whitespace comments (which a linter should handle) or, here, spelling/grammar corrections.
(If everyone in the world except us uses Phabricator to perform dozens of spelling/grammar corrections, that's great, and it's something we can improve -- but this task probably isn't the highest leverage improvement available.)
Do you bucket style guide violations with lint issues? While I obviously can't post internal reviews, I can give you a hand-waving indication that a non-zero percentage of comments are style-related things that aren't caught by linters.
If a linter could theoretically catch it (conventions, space, formatting, etc.), I think a linter should (in a perfect world) be catching it.
From a personal/experience perspective, I think a good style guide should ultimately have this rule, at least in spirit:
If you can't write a linter for it, it can't be part of the style guide.
And then reviewers should decide for themselves if purely stylistic things they have a fondness for are important enough to be worth writing a linter for, or not that important (which means they can't be part of the style guide, and they can't complain about it in review). You can't cover everything with this approach, but my experience at Facebook and in this project is that you can get well into the "good enough" territory and that style feedback will pretty much disappear from the review process and all authors and all reviewers will be much, much happier for it.
Getting there is hard, though. Most third-party linters can't auto-correct and often can't emit individual changes; our first-party AAST stuff is powerful but rough and can't parse most languages; only a handful of languages like Go have tools like gofmt; the whole binding stuff is a mess (T10038); and I think we're probably wandering down a dark path if we bank on ever being able to ship linters that are a good fit for most projects (because of how much diversity there is across projects, even in the same language -- that said, I do think the general AAST-based-linter approach is sufficiently powerful that we could theoretically make inroads here, just no part of it will be very easy.
If this is primarily solving "auto-correcting, style-enforcing lint is hard, and until we can invest in that [which might be "never"] we have to deal with dozens of style comments on every change" I'm completely onboard with framing this as an onramp toward effective lint, and think it's a reasonable stepping stone to reduce the pain of the pre-lint era of code review.
I just want to challenge assumptions like these, which seem to be implicit in some of the scenarios here but which aren't very consistent with my experience:
In all cases I've directly observed where one or more of these is true, it looks to me like there's some bigger root problem and this isn't really addressing it. Some of these root problems are really hard to address, like "we're paying engineers $300K/year to collaborate on correcting whitespace errors, which a computer could do faster and more accurately instead and which everyone hates doing and basically feels bad and a little resentful about", but they're the problems I'd ultimately like our sights to be set on.
For me it would be a great enhancement if there would be a shortcut which temporarily toggles this option:
Settings → Personal Settings → APPLICATIONS → Diff Preferences → Show Older Inlines