Update commit/PR links in template and proposals #749
Conversation
| @@ -3,7 +3,7 @@ | |||
| * Proposal: [SE-0002](0002-remove-currying.md) | |||
| * Author: [Joe Groff](https://github.com/jckarter) | |||
| * Status: **Implemented (Swift 3)** | |||
| * Commit: [apple/swift@983a674](https://github.com/apple/swift/commit/983a674e0ca35a85532d70a3eb61e71a6d024108) | |||
| * Implementation: [apple/swift@983a674](https://github.com/apple/swift/commit/983a674e0ca35a85532d70a3eb61e71a6d024108) | |||
There was a problem hiding this comment.
This is an interesting one. I think there is a difference between the "Implementation" that was used to evaluate the proposal and the final "commit" where the change was integrated into master. Even once there is an implementation that is acceptable for reviewing a proposal it may not be in sufficient stage to just merge into master.
There was a problem hiding this comment.
I added those commit links (to proposals without SR bugs) in case they'd be useful to someone implementing a similar feature.
The original commit for SE-0002 might be swiftlang/swift@30af42f but that's not very useful either.
I can remove the commit links, and/or close this pull request (if you prefer the existing proposal template).
There was a problem hiding this comment.
I think the links are useful. I'm more just thinking about terminology and what they convey. After thinking about this some more, I think this is a good change as is.
0000-template.md
Outdated
| @@ -4,7 +4,7 @@ | |||
| * Authors: [Author 1](https://github.com/swiftdev), [Author 2](https://github.com/swiftdev) | |||
| * Review Manager: TBD | |||
| * Status: **Awaiting review** | |||
| * Pull Request: [apple/swift#NNNNN](https://github.com/apple/swift/pull/NNNNN) | |||
| * Implementation: [apple/swift#NNNNN](https://github.com/apple/swift/pull/NNNNN) | |||
There was a problem hiding this comment.
How about Awaiting implementation instead of a PR link? We are fine with pull requests being created before there is an implementation. They just will not be accepted for formal review until there is one. But having a PR is useful to have a discussion on whether or not the proposal is ready, as well as have a focal point to discuss on swift-evolution the implementation needed for a candidate proposal.
There was a problem hiding this comment.
Like this?
* Implementation: **Awaiting implementation**But if @krilnon needs to parse PR links in a required format, wouldn't it be better to have this in the template?
There was a problem hiding this comment.
As it stands, pull requests that haven't been merged in to master aren't parsed at all — because they haven't started a formal review and therefore don't show up on the status page. So the decision here doesn't matter much from the PoV of the parser. People will make slightly more formatting mistakes if we don't show the link format somewhere, but they can always look at past proposals rather than the template.
I do like the call to action implied by Awaiting Implementation. I think Ben's point is that the template file serves as an answer to the question "what structure do I need in my proposal document?", whereas the process.md file (and readme.md) answers the question "how do I go through the steps of proposing a change?".
There was a problem hiding this comment.
How about this?
# Feature name
* Proposal: [SE-NNNN](NNNN-filename.md)
* Authors: [Author 1](https://github.com/swiftdev), [Author 2](https://github.com/swiftdev)
* Review Manager: TBD
* Status: **Awaiting implementation**
*During the review process, add the following fields as needed:*
* Implementation: [apple/swift#NNNNN](https://github.com/apple/swift/pull/NNNNN)
* Decision Notes: [Rationale](https://lists.swift.org/pipermail/swift-evolution/), [Additional Commentary](https://lists.swift.org/pipermail/swift-evolution/)
* Bugs: [SR-NNNN](https://bugs.swift.org/browse/SR-NNNN), [SR-MMMM](https://bugs.swift.org/browse/SR-MMMM)
* Previous Revision: [1](https://github.com/apple/swift-evolution/blob/...commit-ID.../proposals/NNNN-filename.md)
* Previous Proposal: [SE-XXXX](XXXX-filename.md)In the formulation above, Awaiting Implementation becomes a status, (or pseudo-status because it'll never be parsed), and the Implementation label is moved to the "add these list items as needed" section.
There was a problem hiding this comment.
* Status: **Awaiting implementation**I like this idea.
|
@krilnon I think this is ready to merge, whenever you're ready. @tkremenek Do you need to update process.md for Swift 5, to say that an implementation is now required (instead of encouraged), and to add Awaiting implementation to the list of states? |
|
@benrimmington I changed process.md in f4d73b1 to say that an implementation is required. |
|
Okay, this looks good. Thanks! |
See also #747