Page MenuHomePhabricator
Create Task
Maniphest T6414

Ability to put comments on current code
Closed, WontfixPublic
Actions

Description

Use-case:

  • While browsing the code for something, I find a typo, or a minor problem, and I want to make a quick todo for it, without breaking flow.
  • Gradual changes over many diffs create room code smells, that are more visible from code-browsing view than code-review view.
  • Initial reviewing of a large-ish code-base (For example - when upgrading from proof-of-concept to "real" project) - these may be too large for a single-page revision.

Right now, this would be solved with creating a task, and saying "see foo.c, line 44" (or direct link to revision and line); This is not visible from the code view, and can be hard to track if unrelated changes are made to the file.

Feature:
Attaching a comment to a code lines, that is "live" while the code line "doesn't change".

  • Ideally, this comment would be "alive" while the line(s) don't change (proxy: blame to the same commit), even if the rest of the file is changed.
  • Visible in the diffusion view, similar to how lint messages are supported in diffusion.
  • If a revision/commit is touching these lines (or the file), show the comment on the left side in the review.
  • Some way of seeing all "live" comments in a repo/subpath. Maybe show "this file has comments" in the directory view.
  • easy escalation to a task / something like T1460 / permalink (To connect from external task trackers)

This may be slightly related to T5722.

Related Objects

Event Timeline

avivey created this task.Oct 28 2014, 6:33 PM
avivey raised the priority of this task from to Wishlist.
avivey updated the task description. (Show Details)
avivey added a project: Diffusion.
avivey updated the task description. (Show Details)
avivey added a subscriber: avivey.
btrahan closed this task as Wontfix.Oct 29 2014, 3:24 PM
btrahan claimed this task.

This seems pretty funky to me versus something like T6008 where you could just edit the code as you go, achieving the goal of easily adding "TODO" comments all over the code. I also think the amount of people browsing the code via diffusion versus looking at the actual code itself is such that you're much better off with the comments actually being part of the code. I also had my first waking nightmare about the user confusion as these comments pop up in Differential / Audit views versus something like T6008 where they'd be there in the code itself in a rather sensical fashion.

Otherwise, I think T5722 is what you want. You'll just make an audit that "is the whole codebase" or whatever and everything will work like you want within the context of that audit.

If in a year or two you still think this is interesting and something we should add, please do re-file or re-open this task.

epriestley added a subscriber: epriestley.Oct 29 2014, 3:30 PM

Some specific notes on this:

  • Identifying responsible users is hard. A theme we try to adhere to across applications is having users who are clearly and unambiguously responsible for things. If you drag an inline over 5 lines, and those were most recently touched in 5 different commits, we now have a lot of people implicated. This is bad from a general "getting things done" point of view, and from a technical point of view (who do we mail, how do we make sure the most active committers aren't getting a ton of collateral damage mail, etc).
  • Brining comments forward across changes is really hard, and doing it in a way that users understand what's going on is doubly so. We'll take a crack at this in Differential eventually, and might think about bringing it to Diffusion after that, but this is a long ways away.
  • A workaround is to browse with blame. When you see something sketchy, click to the blamed commit and raise concern (or leave an inline). This is slightly more work for you, but dodges all the issues of porting things forward, ambiguity, etc. Seeing the commit that made the change sometimes provides context which is important in understanding it, too (offhand, for example, if the commit was "this is garbage but we're deleting it tomorrow" you can know that your concern is going to be addressed shortly).
avivey mentioned this in T8273: Discussion about existing source code.May 20 2015, 9:23 PM
jrioux added a subscriber: jrioux.Sep 3 2015, 12:23 PM
Phabricator · Built by Phacility