aaron.ballman marked 2 inline comments as done. aaron.ballman added inline comments.
================ Comment at: llvm/docs/DeveloperPolicy.rst:349 -* If the patch has been reviewed, add a link to its review page, as shown - `here <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`_. ---------------- reames wrote: > aaron.ballman wrote: > > reames wrote: > > > aaron.ballman wrote: > > > > aaron.ballman wrote: > > > > > reames wrote: > > > > > > Removing this item seems very off topic for the change description, > > > > > > and certainly hasn't been discussed in the linked thread. Please > > > > > > add this back in a separate commit. > > > > > > > > > > > > (To be clear, no objections to the overall change, just the removal > > > > > > of the phab link text.) > > > > > Hmm, I thought this was obsoleted by the new text (it is covered by > > > > > "other kinds of metadata"). That said, losing that link is definitely > > > > > a regression, so thank you for pointing this out! I'll find a way to > > > > > add it back in (either as a stand-alone bullet point or incorporated > > > > > into the new text). > > > > I restored the link in > > > > https://github.com/llvm/llvm-project/commit/a1562bbc63b49a70b39ba075d9a3332f50cea11d > > > > as part of the new bullet; please let me know if you have additional > > > > concerns. > > > That 90% covers it. What's left is some minor framing. I'd suggest > > > separating that point into two. The first should be recommended metadata > > > (phab, issues link), and the second can be the additional metadata point. > > > Something like: > > > > > > ``` > > > If the patch has been reviewed, add a link to its review page, as shown > > > `here > > > <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`_. If > > > the patch fixes a bug in GitHub Issues, we encourage adding a reference > > > to the issue being closed, as described `here > > > <https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs>`_. > > > > > > It is also acceptable to add other metadata to the commit message to > > > automate processes, including for downstream consumers. and including > > > links to resources that are not available to the entire community. > > > However, such links and/or metadata should not be used in place of making > > > the commit message self-explanatory. > > > > > > ``` > > > All of the above is just reorganizing what you had written with some very > > > minor copy editing. I'd separately suggest adding the following sentence > > > at the end of the second bullet. > > > > > > Note that such non-public links are *only* allowed in commit messages, > > > and should not be included in the submitted code. > > I did some minor rewording for clarity, so how about: > > ``` > > * If the patch has been reviewed, add a link to its review page, as shown > > `here <https://www.llvm.org/docs/Phabricator.html#committing-a-change>`__. > > If the patch fixes a bug in GitHub Issues, we encourage adding a > > reference to > > the issue being closed, as described > > `here <https://llvm.org/docs/BugLifeCycle.html#resolving-closing-bugs>`__. > > > > * It is also acceptable to add other metadata to the commit message to > > automate > > processes, including for downstream consumers. This metadata can include > > links to resources that are not available to the entire community. > > However, > > such links and/or metadata should not be used in place of making the > > commit > > message self-explanatory. > > ``` > > > > > All of the above is just reorganizing what you had written with some very > > > minor copy editing. I'd separately suggest adding the following sentence > > > at the end of the second bullet. > > > > > > Note that such non-public links are *only* allowed in commit messages, > > > and should not be included in the submitted code. > > > > I think this might need more wordsmithing, which is why I left out of the > > simple reorganization. The non-public links aren't limited to commit > > messages -- for example, they're fine to use in a phabricator review or > > github issue comment, etc. So I don't want to be too restrictive with the > > wording, though I agree with the intent. How about something along the > > lines of: > > > > Note that such non-public links should not be included in the submitted > > code. > LGTM to both parts. Wording is hard, and good catch on the second. :) Thank you for the help! Because this is rearranging existing text and the additional text is a minor point of clarity, I'll land those changes shortly -- but as always, if folks have more post-commit review feedback on those changes, please speak up and I'll work with you on it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155081/new/ https://reviews.llvm.org/D155081 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
