Skip to content
This repository has been archived by the owner on Nov 27, 2020. It is now read-only.

WIP: New command syntax #6

Closed
wants to merge 2 commits into from
Closed

WIP: New command syntax #6

wants to merge 2 commits into from

Conversation

z0al
Copy link
Owner

@z0al z0al commented Dec 29, 2017

This PR introduces a new command syntax as an alternative to /depends and /ensure commands. The new syntax is inspired by GitHub issue closing patterns.

To use the new syntax one would simply write, for example:

depends on #1, #2, and owner/repo#3

NOTES:

  • owner/repo#3 won't work (yet), because we ONLY support dependencies within the same repository. However, this might change in the future (Support for cross-repository dependencies #2)
  • The keyword depend could be written as: depend, depends, depended, or depending
  • Depend command may - optionally - be followed by on
  • ensure keyword isn't supported

The old syntax should continue to work for a while, but it's deprecated. We may also notify users via PR comments if we add support for them.

To-Dos:

Resolves #3 . /CC @nesl247 @ryanhiebert

@nesl247
Copy link
Contributor

nesl247 commented Dec 29, 2017

Looks awesome to me!

@ryanhiebert
Copy link
Contributor

ryanhiebert commented Dec 30, 2017

GitHub's keywords for closing pull requests only allow the keyword to affect a single issue reference. I think this is a pattern that it would be best to follow. For instance, if the dependencies followed the pattern:

Depends on #17, #18

only #17 would be a dependency. To make both of them dependencies, you'd need to write

Depends on #17. Depends on #18

This matches they way that GitHub requires for closing keywords, to avoid closing issues accidentally when they are only intended to be references:

Closes #12, #13


When using keywords, it may make sense to only work with those in the original issue description, and ignore anything that is in follow-up comments. This both matches what GitHub does with its keywords: they must appear in the pull request description (the first comment).

For slash commands, you'd want them to be available in any comment (including the description), but for keywords I think it probably makes sense to follow GitHub's lead here and make them only work in the description. It also helps deal with access control by default, because GitHub only allows maintainers and the OP to update the description.

@ryanhiebert
Copy link
Contributor

I'd also recommend dropping matching on the *ing suffix, because it doesn't match the pattern that GitHub has for its keyword, that only include the fix, fixes, and fixing forms for fix.

https://help.github.com/articles/closing-issues-using-keywords/

@z0al
Copy link
Owner Author

z0al commented Jan 1, 2018

This matches they way that GitHub requires for closing keywords, to avoid closing issues accidentally when they are only intended to be references

GitHub allows closing keywords on both description and commit messages. For commits, yes it makes sense, but for PR description I don't think it's a practical problem.

When using keywords, it may make sense to only work with those in the original issue description, and ignore anything that is in follow up comments.

This is actually intelligent solution, not just it would eliminate the need for probot-metadata but also makes things more clear (no need to scroll down to see comments) and of course - as you mentioned -it would handle permissions by default, excellent @ryanhiebert 👍

@ryanhiebert
Copy link
Contributor

GitHub allows closing keywords on both description and commit messages.

My suggestion on that front isn't so much to solve a problem, but to avoid inconsistencies with the feature we're trying to match, so that people that understand the keyword feature aren't confused about it.

@z0al
Copy link
Owner Author

z0al commented Jan 20, 2018

I will go with @ryanhiebert suggestions but in a new PR, closing this ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants