Skip to content
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

feat: porting README generation from nodejs-repo-tools #206

Merged
merged 11 commits into from
Mar 26, 2019
Merged

feat: porting README generation from nodejs-repo-tools #206

merged 11 commits into from
Mar 26, 2019

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Mar 21, 2019

I'm midway through porting over the logic from nodejs-repo-tools for generating samples/README.md, and ./README.md.

This process was made easier by the fact that @busunkim96 has been working to define a better meta-information file for our repos, which looks like this:

{
        "name": "storage",
        "name_pretty": "Google Cloud Storage",
        "product_documentation": "https://cloud.google.com/storage", 
        "client_documentation": "https://cloud.google.com/nodejs/docs/reference/storage/2.3.x/",
        "issue_tracker": "https://issuetracker.google.com/savedsearches/559782",
        "release_level": "ga",
        "language": "nodejs",
        "repo": "googleapis/nodejs-storage",
        "distribution_name": "@google-cloud/storage"
}

☝️ prior to this effort, there was a standard way to fetch a lot of this information.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 21, 2019
@@ -0,0 +1,117 @@
[//]: # "This README.md file is auto-generated, all changes to this file will be lost."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jkwlui jkwlui requested a review from busunkim96 March 21, 2019 21:52
@JustinBeckwith
Copy link
Contributor

So here's my 2 cents. We should integrate and ship this ASAP, because it will help us drop an unmaintained dependency in nodejs-repo-tools. Moving forward, I'm assuming you're both working towards a common metadata format :)

# @returns {string} The markdown badge.
def release_quality_badge(input):
if input:
return "hello world"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a place holder? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've replaced this with the real thing now.

@@ -20,6 +20,9 @@
from synthtool import _tracked_paths
from synthtool import metadata

import json
import os

Copy link
Contributor

Choose a reason for hiding this comment

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

Could these two imports be put above from pathlib import Path?

badge = "general%20availability%20%28GA%29-brightgreen"
elif release_quality == "BETA":
badge = "beta-yellow"
elif release_quality == "ALPHA":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@busunkim96 went ahead and ported the logic from JavaScript; still have some work to do to load samples from disk.

@bcoe bcoe changed the title [WIP] feat: porting README generation from nodejs-repo-tools feat: porting README generation from nodejs-repo-tools Mar 26, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Mar 26, 2019

@busunkim96 @JustinBeckwith (CC: @jmdobry for some context from today's conversation). I've finished implementing the README generation logic and added tests. Here's an example of the rendering:

https://gist.github.com/bcoe/9014b958aaec4553d2302e0e55f3d17e

## Using the client library

1. [Select or create a Cloud Platform project][projects].
1. [Enable billing for your project][billing].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we used to be able to hide this step for libraries that don't require billing; do we still need this @JustinBeckwith, how common is this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some sleuthing about, and Firestore is the only one I see using it:
https://github.com/googleapis/nodejs-firestore/blob/master/.cloud-repo-tools.json#L6

That having been said - there are plenty of our products that have a free tier:
https://cloud.google.com/free/

My 2 cents: I would like to see us keep this field, and actually set it correctly, because I think requiring billing is a huge leap for a lot of developers that just want to play on the platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

(also sorry for creating work)

Copy link
Contributor

Choose a reason for hiding this comment

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

Quickstarts on cloud.google.com seem to always have the billing step (even products listed at cloud.google.com/free. Maybe our READMEs should also always have this step, for consistency?

As an aside, it kind of makes sense that the billing step was suppressed for firestore, since their Get Started doesn't explicitly mention it. 🤔

[shell_img]: https://gstatic.com/cloudssh/images/open-btn.png
[projects]: https://console.cloud.google.com/project
[billing]: https://support.google.com/cloud/answer/6293499#enable-billing
[enable_api]: https://console.cloud.google.com/flows/enableapi?apiid={{ metadata['repo']['api_id'] }}
Copy link
Contributor Author

@bcoe bcoe Mar 26, 2019

Choose a reason for hiding this comment

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

my goodness, I can't believe I spent 20 minutes today reading this as app_id; sorry for inflicting my 🧠 💤 on you @jmdobry / @busunkim96.

@bcoe bcoe merged commit ee98b8b into master Mar 26, 2019
@bcoe bcoe deleted the readmes branch March 26, 2019 20:22
@tseaver tseaver mentioned this pull request Mar 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants