chore: remove name_pretty_override from google-cloud-maintenance-api#16842
chore: remove name_pretty_override from google-cloud-maintenance-api#16842jskeet merged 2 commits intogoogleapis:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request renames the product from "Maintenance API" to "Maintenance" across metadata and documentation files, including READMEs and configuration files. The review feedback highlights several instances where the resulting phrasing is grammatically awkward or ambiguous, specifically suggesting the use of "Enable Maintenance" instead of "Enable the Maintenance" and maintaining "Maintenance API" in contexts where referring to the service versions for better clarity.
| 1. `Select or create a Cloud Platform project.`_ | ||
| 2. `Enable billing for your project.`_ | ||
| 3. `Enable the Maintenance API.`_ | ||
| 3. `Enable the Maintenance.`_ |
There was a problem hiding this comment.
The phrase "Enable the Maintenance" is grammatically awkward. It should be "Enable Maintenance" or "Enable the Maintenance API". Since the goal of this change appears to be removing "API" from the product name, "Enable Maintenance" is a better fit.
| 3. `Enable the Maintenance.`_ | |
| 3. Enable Maintenance._ |
| .. _Select or create a Cloud Platform project.: https://console.cloud.google.com/project | ||
| .. _Enable billing for your project.: https://cloud.google.com/billing/docs/how-to/modify-project#enable_billing_for_a_project | ||
| .. _Enable the Maintenance API.: https://cloud.google.com/unified-maintenance/ | ||
| .. _Enable the Maintenance.: https://cloud.google.com/unified-maintenance/ |
There was a problem hiding this comment.
This link reference should be updated to match the suggested change in the list item for consistency.
| .. _Enable the Maintenance.: https://cloud.google.com/unified-maintenance/ | |
| .. _Enable Maintenance.: https://cloud.google.com/unified-maintenance/ |
| 1. `Select or create a Cloud Platform project.`_ | ||
| 2. `Enable billing for your project.`_ | ||
| 3. `Enable the Maintenance API.`_ | ||
| 3. `Enable the Maintenance.`_ |
There was a problem hiding this comment.
The phrase "Enable the Maintenance" is grammatically awkward. It should be "Enable Maintenance" or "Enable the Maintenance API". Since the goal of this change appears to be removing "API" from the product name, "Enable Maintenance" is a better fit.
| 3. `Enable the Maintenance.`_ | |
| 3. Enable Maintenance._ |
| .. _Select or create a Cloud Platform project.: https://console.cloud.google.com/project | ||
| .. _Enable billing for your project.: https://cloud.google.com/billing/docs/how-to/modify-project#enable_billing_for_a_project | ||
| .. _Enable the Maintenance API.: https://cloud.google.com/unified-maintenance/ | ||
| .. _Enable the Maintenance.: https://cloud.google.com/unified-maintenance/ |
There was a problem hiding this comment.
This link reference should be updated to match the suggested change in the list item for consistency.
| .. _Enable the Maintenance.: https://cloud.google.com/unified-maintenance/ | |
| .. _Enable Maintenance.: https://cloud.google.com/unified-maintenance/ |
| .. include:: multiprocessing.rst | ||
|
|
||
| This package includes clients for multiple versions of Maintenance API. | ||
| This package includes clients for multiple versions of Maintenance. |
There was a problem hiding this comment.
The phrase "versions of Maintenance" is ambiguous and sounds like it refers to maintenance events rather than the service. Using "versions of the Maintenance API" or "versions of the Maintenance service" would be much clearer.
| This package includes clients for multiple versions of Maintenance. | |
| This package includes clients for multiple versions of the Maintenance API. |
jskeet
force-pushed
the
maintenance-package
branch
from
9866d71 to
f109097
Compare
|
@parthea Using an additional string replacement for docs isn't nice, but it does isolate the solution to where the problem actually occurs. (The "name pretty" being Mainteance isn't inherently an issue.) |
| packages/google-cloud-maintenance-api/README.rst | ||
| ] | ||
| before: '_Maintenance:' | ||
| after: '_Cloud Maintenance:' |
There was a problem hiding this comment.
Would it be better to keep the override rather than add post processing here?
There was a problem hiding this comment.
I don't think so, because it's really not that we want to override the "pretty name" in general - we just want to fix the link. Aside from anything else, this is a whole option that will be blocked from removal (I believe) based on just this API.
There was a problem hiding this comment.
Ack. Adding post processing is a slippery slope. We should file a bug to follow up on this. In the meantime I'll approve it so that we don't want to block the overall migration effort.
|
Will resolve the conflicts in this PR. |
jskeet
force-pushed
the
maintenance-package
branch
from
f109097 to
27d1cb9
Compare
A string replacement is required to avoid duplicate link IDs in generated READMEs.
jskeet
force-pushed
the
maintenance-package
branch
from
27d1cb9 to
6bfa174
Compare
|
When we come to fix this later, we could decide to use a different title for the present-in-all-readmes "maintenance" link instead, of course. That would remove the need for any string manipulation. (But I don't want to start messing with synthtool templates at this point.) |
| packages/google-cloud-maintenance-api/README.rst | ||
| ] | ||
| before: '_Maintenance:' | ||
| after: '_Cloud Maintenance:' |
There was a problem hiding this comment.
Ack. Adding post processing is a slippery slope. We should file a bug to follow up on this. In the meantime I'll approve it so that we don't want to block the overall migration effort.
2 participants
No description provided.