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: > > 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. 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
