Skip to content

Support standard path variable replacement syntax#3536

Merged
mislav merged 2 commits intocli:trunkfrom
rneatherway:rneatherway/placeholder-syntax
Apr 30, 2021
Merged

Support standard path variable replacement syntax#3536
mislav merged 2 commits intocli:trunkfrom
rneatherway:rneatherway/placeholder-syntax

Conversation

@rneatherway
Copy link
Contributor

Add support for the following synonyms:

{owner} for :owner
{repo} for :repo
{branch} for :branch

If accepted in principle I would like to also update the example docs in this file to use the {var} syntax, perhaps with a note that :var is also supported. I would also extend the test cases.

The motivation for this change is that :var is a Rails-ism, and {var} is more standard. I was particularly motivated to submit this as docs.github.com changed to use {var}, which means that it is not currently possible to cut and paste a URL from the API docs (e.g. Code Scanning) right to the command line.

@cliAutomation
Copy link
Collaborator

Hi! Thanks for the pull request. Please ensure that this change is linked to an issue by mentioning an issue number in the description of the pull request. If this pull request would close the issue, please put the word 'Fixes' before the issue number somewhere in the pull request body. If this is a tiny change like fixing a typo, feel free to ignore this message.

@mislav
Copy link
Contributor

mislav commented Apr 29, 2021

This is great; thank you!

Please do update the documentation to use {} instead of :. In fact, let's not document the : syntax anymore, but obviously let's keep it supported for backwards compatibility.

@mislav mislav self-assigned this Apr 29, 2021
@rneatherway
Copy link
Contributor Author

I've updated the docs and the tests, except for the greedy substitution test. The existing pattern uses \b to look for the end of the replacement text, which doesn't work with {var} because it ends with a non-ASCII character. How important is that behaviour?

rneatherway and others added 2 commits April 30, 2021 14:22
Add support for the following synonyms:

{owner} for :owner
{repo} for :repo
{branch} for :branch
@mislav
Copy link
Contributor

mislav commented Apr 30, 2021

How important is that behaviour?

Since the {...} syntax has a trailing delimiter, you don't have to write any special tests for greediness. That only applies to the old :placeholder syntax.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@mislav mislav merged commit 011e455 into cli:trunk Apr 30, 2021
@rneatherway rneatherway deleted the rneatherway/placeholder-syntax branch April 30, 2021 12:58
@rneatherway
Copy link
Contributor Author

Thanks for the quick review :)

@mrcool1661
Copy link

Tambahkan sokongan untuk sinonim berikut:

{owner} untuk: pemilik
{repo} untuk: repo
{branch} untuk: cabang

Sekiranya diterima pada prinsipnya saya ingin juga mengemas kini contoh dokumen dalam fail ini untuk menggunakan {var}sintaks, mungkin dengan nota yang :varjuga disokong. Saya juga akan melanjutkan kes ujian.

Motivasi untuk perubahan ini adalah bahawa :varadalah Rails-ism, dan {var}adalah lebih standard . Saya sangat terdorong untuk mengirimkannya sebagai docs.github.com berubah menjadi penggunaan {var}, yang bermaksud bahawa saat ini tidak mungkin untuk memotong dan menampal URL dari dokumen API (misalnya Pengimbasan Kod ) tepat ke baris perintah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants