Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions describe.py
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ def document_collection_recursive(


def document_api(
name, version, uri, doc_destination_dir, artifact_destination_dir=DISCOVERY_DOC_DIR
name, version, uri, doc_destination_dir, artifact_destination_dir=DISCOVERY_DOC_DIR, discovery_uri_template=None
):
"""Document the given API.

Expand All @@ -402,14 +402,23 @@ def document_api(
documentation should be saved.
artifact_destination_dir (str): relative path where the discovery
artifacts should be saved.
discovery_uri_template (str): URI template of the API's discovery document.
If this parameter is set, the `uri` parameter is ignored and the uri
will be created from this template.
"""
http = build_http()
resp, content = http.request(
uri
or uritemplate.expand(
# Use the discovery_uri_template to create the uri if provided
if discovery_uri_template:
uri = uritemplate.expand(
discovery_uri_template, {"api": name, "apiVersion": version}
)

if not uri:
uritemplate.expand(
FLAGS.discovery_uri_template, {"api": name, "apiVersion": version}
Copy link

@chingor13 chingor13 Jul 17, 2024

Choose a reason for hiding this comment

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

This seems like an anti-pattern (depending on a global) that can be cleaned up in the future. Then the uritemplate could be always passed in

Copy link
Contributor Author

@parthea parthea Jul 17, 2024

Choose a reason for hiding this comment

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

The order for constructing uri is use discovery_uri_template if provided, then use uri argument if provided, then fallback to FLAGS.discovery_uri_template

It's not clear how this can be cleaned up to preserve this order without another parameter to specify whether to use uri or discovery_uri_template

Copy link
Contributor

@ohmayr ohmayr Jul 17, 2024

Choose a reason for hiding this comment

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

We could instead do something like this:

uri = (discovery_uri_template and uritemplate.expand(discovery_uri_template, {"api": name, "apiVersion": version})) 
            or uri or uritemplate.expand(FLAGS.discovery_uri_template, {"api": name, "apiVersion": version})

Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment below which touches on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7a35f8c

)
)

http = build_http()
resp, content = http.request(uri)

if resp.status == 200:
discovery = json.loads(content)
Expand Down Expand Up @@ -494,6 +503,7 @@ def generate_all_api_documents(
directory_uri=DIRECTORY_URI,
doc_destination_dir=BASE,
artifact_destination_dir=DISCOVERY_DOC_DIR,
discovery_uri_template=None,
):
"""Retrieve discovery artifacts and fetch reference documentations
for all apis listed in the public discovery directory.
Expand All @@ -503,6 +513,7 @@ def generate_all_api_documents(
documentation should be saved.
artifact_destination_dir (str): relative path where the discovery
artifacts should be saved.
discovery_uri_template (str): URI template of the API's discovery document.
"""
api_directory = collections.defaultdict(list)
http = build_http()
Expand All @@ -516,6 +527,7 @@ def generate_all_api_documents(
api["discoveryRestUrl"],
doc_destination_dir,
artifact_destination_dir,
discovery_uri_template,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having document_api not take an extra parameter:

            document_api(
                api["name"],
                api["version"],
                discovery_uri_template or FLAGS.discovery_uri_template or api["discoveryRestUrl"],
                doc_destination_dir,
                artifact_destination_dir,              
            )

Then, in document_api, you can simply have

uri = uri.expand(...)

without any conditionals.

Note also that I have FLAGS.discovery_uri_template in the second position; that makes more sense, I think. But I also think we shouldn't be referencing FLAGS directly from within the functions, but it should be passed in.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I'm not sure why the flag --discovery_uri_template still defaults to DISCOVERY_URI. I think we should change the latter symbol to the value of DISCOVERY_URI_TEMPLATE that you have in scripts/updatediscoveryartifacts.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Please could you take another look?

)
api_directory[api["name"]].append(api["version"])

Expand Down
3 changes: 2 additions & 1 deletion noxfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,14 @@ def scripts(session):
session.install("-r", "scripts/requirements.txt")

# Run py.test against the unit tests.
# TODO(https://github.com/googleapis/google-api-python-client/issues/2132): Add tests for describe.py
session.run(
"py.test",
"--quiet",
"--cov=scripts",
"--cov-config=.coveragerc",
"--cov-report=",
"--cov-fail-under=91",
"--cov-fail-under=90",
"scripts",
*session.posargs,
)
5 changes: 5 additions & 0 deletions scripts/updatediscoveryartifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import describe

SCRIPTS_DIR = pathlib.Path(__file__).parent.resolve()
# Obtain the discovery index and artifacts from googleapis/discovery-artifact-manager
DIRECTORY_URI = "https://raw.githubusercontent.com/googleapis/discovery-artifact-manager/master/discoveries/index.json"
DISCOVERY_URI_TEMPLATE = "https://raw.githubusercontent.com/googleapis/discovery-artifact-manager/master/discoveries/{api}.{apiVersion}.json"
DISCOVERY_DOC_DIR = (
SCRIPTS_DIR / ".." / "googleapiclient" / "discovery_cache" / "documents"
)
Expand All @@ -46,8 +49,10 @@

# Download discovery artifacts and generate documentation
describe.generate_all_api_documents(
directory_uri=DIRECTORY_URI,
doc_destination_dir=REFERENCE_DOC_DIR,
artifact_destination_dir=DISCOVERY_DOC_DIR,
discovery_uri_template=DISCOVERY_URI_TEMPLATE,
)

# Get a list of files changed using `git diff`
Expand Down