reames 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>`_. ---------------- 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. :) 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
