[PATCH] D155081: Specify the developer policy around links to external resources

[Aaron Ballman via Phabricator via cfe-commits]

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