Conversation
summer-ji-eng
left a comment
There was a problem hiding this comment.
Try to understand what's the motivation of removing the markdown. Maybe I miss something in chat.
| {%- endfor -%} | ||
| {%- endmacro -%} | ||
|
|
||
| {%- macro markDownStuff(formattedLine) %} |
There was a problem hiding this comment.
Could you rename markDownStuff to some name readable? 😆
And looks like this method has not been used.
There was a problem hiding this comment.
Sorry! This was some old code I ended up deleting. Thanks for the review :)
| {%- if trimmed.length > 0 %} | ||
| * {{ trimmed.replaceAll('*/', '* /') | safe }} | ||
| {%- if trimmed.length > 0 -%} | ||
| {%- set regExp = r/\[(.*?)\]\(.*?\)/g -%} |
There was a problem hiding this comment.
This line is similar to line 32, is that possible we could reuse the regExp
f0e7546 to
2d606cb
Compare
|
Hey @summer-ji-eng! Thanks for the review, I ended up doing some more major changes to this PR since there were some markdowns that were left behind. The impetus for removing markdowns is in googleapis/google-cloud-node-core#594, I think it's just a preference for the generated samples since it won't actually link to anything. |
bcoe
left a comment
There was a problem hiding this comment.
Left a nit, looks good to me in general though.
| * "folders/[FOLDER_ID]" | ||
| * "projects/PROJECT_ID" | ||
| * "organizations/ORGANIZATION_ID" | ||
| * "billingAccounts/BILLING_ACCOUNT_ID" |
There was a problem hiding this comment.
It's a little weird that we change the format of billingAccounts/[BILLING_ACCOUNT_ID] to billingAccounts/BILLING_ACCOUNT_ID, since this isn't markdown, rather it's an indicator of the value we need to replace.
This is a minor nit, because I think billingAccounts/BILLING_ACCOUNT_ID conveys the same things, "You must replace this variable".
If you decided you wanted to fix this nit, perhaps you could look at whether the [ is inside a quoted string?
There was a problem hiding this comment.
I personally prefer to use original format quote inside of [. As developer, it is more obvious to notice the variable is need to be replace and it is different from billingAccounts.
Additionally, the original format is consistent with other languages as well.
| * and [Resource Name | ||
| * Format](https://cloud.google.com/asset-inventory/docs/resource-name-format) | ||
| * See Resource | ||
| * Names (https://cloud.google.com/apis/design/resource_names#full_resource_name) |
There was a problem hiding this comment.
The link is clickable. Sorry could you remind me what is the initiative to remove the markdown links here?
See [Resource
* Names](https://cloud.google.com/apis/design/resource_names#full_resource_name)
Looks better IMO.
baselines/kms/samples/generated/v1/key_management_service.asymmetric_sign.js.baseline
Show resolved
Hide resolved
cc6d04e to
c4d27e2
Compare
fhinkel
left a comment
There was a problem hiding this comment.
LGTM. Since this is all in JS files and not in TS comments, I think it's good this way.
Fixes googleapis/google-cloud-node-core#594