Keep inline comments that have no text and render their suggestions - #6700
Open
daogottwald wants to merge 1 commit into
Open
Keep inline comments that have no text and render their suggestions#6700daogottwald wants to merge 1 commit into
daogottwald wants to merge 1 commit into
Conversation
Fixes mozilla#6699. An inline comment points at a file and a line, so it is worth showing even when the reviewer left the text empty; a general comment with no text is not. `transaction.search` reports a suggestion-only inline with `content.raw: ""`, which the previous filter discarded before anything read its fields. `hasSuggestion`, `suggestionText` and `isNewFile` reach the deployed Phabricator with mozilla-conduit/phabricator#102, so the suggestion block and the resolved `on_removed_code` stay inert until then.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6699.
_all_commentsfiltered onc.content.strip(), so an inline comment whose text is empty was discarded beforePhabricatorInlineCommentever readtransaction["fields"]. A "Suggest Edit" inline where the reviewer put everything into the suggestion hascontent.raw: "", so it disappeared fromto_md()entirely -- no header, no file, no line -- and a reader ofget_phabricator_revisioncould not tell a review comment was left there.An inline comment points at a file and a line, so it is worth showing even with no text; a general comment with no text is not. That split is now
is_renderable. Removed comments are dropped explicitly, since the text filter used to do that incidentally (Phabricator blanks the content of a removed comment).While here, the two inline fields from mozilla-conduit/phabricator#102 are picked up:
suggestionTextrenders as a fencedsuggestionblock, andisNewFileresolveson_removed_code, which currently sendsget_matching_hunkdown its guess-the-side heuristic. Both are read with.get(), so they stay inert until that change is deployed and nothing breaks on a Phabricator that lacks them._sanitize_commentsclearssuggestion_textalongsidecontent, so an untrusted author's suggestion cannot reach a model prompt through the new field.Testing
tests/test_phabricator.pygains unit coverage for each branch plus an end-to-endto_md()test for the reported symptom; the latter fails without the fix.Locally: 325 passed across
tests/(excludingtest_nlp.py, which needs thenlpextra). Thetest_repository.pyfailures on my machine are environmental -- nohg, norust-code-analysis-web-- and reproduce identically onmaster.