-
Notifications
You must be signed in to change notification settings - Fork 555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update synth.py to add AUTHENTICATION.md to file lists in *.gemspec and .yardopts #3202
Conversation
If this looks good for google-cloud-asset, I will copy the change to the other generated packages. This change will not be copied to the manual/veneer packages. |
Should this change be made to #3171 instead? |
I was thinking it will be easier to understand and track our changes to synth.py if we keep them separate from generated PRs. |
Add AUTHENTICATION.md to file lists in *.gemspec and .yardopts [refs googleapis#2933]
google-cloud-asset/synth.py
Outdated
@@ -133,5 +133,16 @@ def escape_braces(match): | |||
]) | |||
) | |||
|
|||
s.replace( | |||
'google-cloud-asset.gemspec', | |||
'"README.md"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, these synth hacks are to be present until we fix it upstream, so I've generally been writing them so that when it does get fixed upstream, it doesn't break. For example, instead of replacing "README.md"
with "README.md", "AUTHENTICATION.md"
, I'd replace "README.md", "LICENSE"
with "README.md", "AUTHENTICATION.md", "LICENSE"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
google-cloud-asset/synth.py
Outdated
s.replace( | ||
'.yardopts', | ||
'README.md', | ||
'README.md\nAUTHENTICATION.md' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, I'd suggest replacing README.md\n
with README.md\nAUTHENTICATION.md\n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. I also found 2 gems in which LICENSE
was being added to .yardopts
, and in these files I merged the older operation with this one.
@dazuma Look OK to merge now? |
Add AUTHENTICATION.md to file lists in *.gemspec and .yardopts
For background, see: googleapis/synthtool#225
[refs #2933]
Example result: