feat!: retrieve all markdown pages #266

dandhlee · 2022-11-14T09:39:21Z

Recovered all markdown pages in nested directory structures. This was not correctly retrieved as I was only looking in the top directory.

Few things that's happening (happy to break down, but was hard to do so to have all the moving parts in one picture):

Keeping track of where pages are retrieved. This is vital as we need to know where markdown pages will need to end up in the TOC
Merging the Markdown and Package's TOC, based on the two sources pulled separately. I could choose to just move all the markdown files at the root directory, but I think a better experience will be to put the markdown pages where they are located.
Remove unused markdown pages that are generated. These are redundant API pages that Sphinx generated but are not used by DocFX Yaml plugin.

Only the handwritten TOC is updated and works as expected, gapic-combo and gapic-auto doesn't contain any markdown pages in nested directories.

Some markdown pages might come with a mix of handwritten guides and autogenerated API content. I plan on working with the library maintainers to have this updated throughout (<10) for the purpose of this plugin that separates Sphinx HTML and DocFX YAML content.

Breaking Change: Upgrading to using 3.9. All Python client libraries are able to use 3.9, so this is not a problem, but will be a backwards incompatible change.

Fixes #217.

Tests pass

dandhlee · 2022-11-14T09:40:27Z

tests/testdata/goldens/handwritten/modules.md

@@ -0,0 +1,52 @@
+# Modules for Python Storage


This file is generated as part of the library's structure for handwritten library. We shouldn't use this in production, but WAI with the way that the directory is set up. In practice, this will need to be removed in the actual library and have the Sphinx config re-structured.

tbpg · 2022-11-14T18:41:54Z

docfx_yaml/extension.py

+    pkg_toc_queue = [package for package in pkg_toc_yaml]
+    for entry in pkg_toc_queue:


Use list(pkg_toc_yaml) to create a copy?

tbpg · 2022-11-14T18:42:44Z

docfx_yaml/extension.py

+    pkg_toc_queue = [package for package in pkg_toc_yaml]
+    for entry in pkg_toc_queue:
+        if (children := entry.get('items')):
+            pkg_toc_queue.extend(children)


This modifies the list being iterated over, which is generally bad. Consider something like flatten_items which can be recursive and flattens the list of items.

Done. Added _flatten_tocs entry to address this.

tbpg · 2022-11-14T19:22:03Z

docfx_yaml/markdown_utils.py

+    base_markdown_dir = Path(app.builder.outdir).parent / "markdown"
+
+    markdown_dir = (
+        base_markdown_dir / '/'.join(cwd)


Suggested change

base_markdown_dir / '/'.join(cwd)

base_markdown_dir.joinpath(*cwd)

(Using '/' and pathlib at the same time is a red flag.)

Done. Thank you for this!

tbpg · 2022-11-14T19:26:45Z

tests/testdata/goldens/handwritten/toc.yml

@@ -4,6 +4,16 @@
  - href: changelog.md
    name: Changelog
  - items:
+    - href: blobs.md


Do these test files need to be as long as they are?

The content should be separated, but works as intended - blobs and buckets should be filtered but there is no class entry with the exact name, but blob and bucket exists. This is the case for ACL and other libraries: there is a markdown file generated but not moved over as we see that there is an entry for which it exists.

See blobs versus ACL: similar entries but blobs is not filtered out and ACL is filtered.

dandhlee

Thank you! Please take a look again.

dandhlee · 2022-11-15T06:29:56Z

docfx_yaml/extension.py

+    pkg_toc_queue = [package for package in pkg_toc_yaml]
+    for entry in pkg_toc_queue:


dandhlee · 2022-11-15T06:30:54Z

docfx_yaml/markdown_utils.py

+    base_markdown_dir = Path(app.builder.outdir).parent / "markdown"
+
+    markdown_dir = (
+        base_markdown_dir / '/'.join(cwd)


Done. Thank you for this!

dandhlee · 2022-11-15T06:32:27Z

tests/testdata/goldens/handwritten/toc.yml

@@ -4,6 +4,16 @@
  - href: changelog.md
    name: Changelog
  - items:
+    - href: blobs.md


The content should be separated, but works as intended - blobs and buckets should be filtered but there is no class entry with the exact name, but blob and bucket exists. This is the case for ACL and other libraries: there is a markdown file generated but not moved over as we see that there is an entry for which it exists.

dandhlee · 2022-11-15T06:33:34Z

tests/testdata/goldens/handwritten/toc.yml

@@ -4,6 +4,16 @@
  - href: changelog.md
    name: Changelog
  - items:
+    - href: blobs.md


See blobs versus ACL: similar entries but blobs is not filtered out and ACL is filtered.

dandhlee · 2022-11-15T07:22:31Z

docfx_yaml/extension.py

+    pkg_toc_queue = [package for package in pkg_toc_yaml]
+    for entry in pkg_toc_queue:
+        if (children := entry.get('items')):
+            pkg_toc_queue.extend(children)


Done. Added _flatten_tocs entry to address this.

tbpg · 2022-11-15T13:27:27Z

docfx_yaml/extension.py

+                toc_queue.extend(_flatten_toc(children))
+                toc_queue.extend(children)


This is still modifying toc_queue while looping over it. Can you iterate over toc_yaml_entry instead?

If I'm understanding correctly, we don't need a queue at all. Just need to iterate over the "graph" and put it all into a single list.

Oops 🤦 Made updates.

dandhlee · 2022-11-15T13:58:33Z

Please take a look again!

tbpg · 2022-11-15T14:01:50Z

docfx_yaml/extension.py

+        for entry in toc_yaml_entry:
+            if (children := entry.get('items')):
+                entries.extend(_flatten_toc(children))
+                entries.extend(children)


Does this line end up double-adding? _flatten_toc is called with children, which will then return children (and more), then we add children again right here.

Seems like this needs a test.

🤦🤦 Yes you're right. Removed the redundant line, added unit test coverage as well. I didn't add it before because it required sphinx.app ... before, but not anymore so fine to add now.

dandhlee · 2022-11-15T16:56:56Z

Please take a look again!

dandhlee added 5 commits November 14, 2022 09:03

feat!: retrieve all markdown files in directories

495f32f

test: add unit test coverage

f5047e5

feat!: require python3.9 and above

427371c

feat: cover all package versions

d8dbd0c

test: update goldens

365a58b

dandhlee requested review from a team as code owners November 14, 2022 09:39

product-auto-label bot added the size: xl Pull request size is extra large. label Nov 14, 2022

test: update unittest

bf3bb93

dandhlee commented Nov 14, 2022

View reviewed changes

tbpg requested changes Nov 14, 2022

View reviewed changes

dandhlee mentioned this pull request Nov 15, 2022

Update docfx minimum Python version to 3.9 on client libraries #267

Open

feat: address review comments

f9999f0

dandhlee commented Nov 15, 2022

View reviewed changes

dandhlee requested a review from tbpg November 15, 2022 07:26

tbpg reviewed Nov 15, 2022

View reviewed changes

dandhlee and others added 2 commits November 15, 2022 08:48

Merge branch 'main' into retrieve_directories

1295fea

feat: address review comment

c40ff7e

dandhlee requested a review from tbpg November 15, 2022 13:58

tbpg reviewed Nov 15, 2022

View reviewed changes

dandhlee added 2 commits November 15, 2022 16:54

feat: address review comment

8f528bd

test: update unit test

d06c5c6

dandhlee requested a review from tbpg November 15, 2022 16:56

tbpg approved these changes Nov 15, 2022

View reviewed changes

dandhlee merged commit 1cee1ed into main Nov 15, 2022

dandhlee deleted the retrieve_directories branch November 15, 2022 19:48

release-please bot mentioned this pull request Nov 15, 2022

chore(main): release 2.0.0 #263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: retrieve all markdown pages #266

feat!: retrieve all markdown pages #266

dandhlee commented Nov 14, 2022

dandhlee Nov 14, 2022

tbpg Nov 14, 2022

dandhlee Nov 15, 2022

tbpg Nov 14, 2022

dandhlee Nov 15, 2022

tbpg Nov 14, 2022

dandhlee Nov 15, 2022

tbpg Nov 14, 2022

dandhlee Nov 15, 2022

dandhlee Nov 15, 2022

dandhlee left a comment

dandhlee Nov 15, 2022

dandhlee Nov 15, 2022

dandhlee Nov 15, 2022

dandhlee Nov 15, 2022

dandhlee Nov 15, 2022

tbpg Nov 15, 2022

dandhlee Nov 15, 2022

dandhlee commented Nov 15, 2022

tbpg Nov 15, 2022

dandhlee Nov 15, 2022

dandhlee commented Nov 15, 2022

		pkg_toc_queue = [package for package in pkg_toc_yaml]
		for entry in pkg_toc_queue:

	base_markdown_dir / '/'.join(cwd)
	base_markdown_dir.joinpath(*cwd)

		toc_queue.extend(_flatten_toc(children))
		toc_queue.extend(children)

feat!: retrieve all markdown pages #266

feat!: retrieve all markdown pages #266

Conversation

dandhlee commented Nov 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandhlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandhlee commented Nov 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandhlee commented Nov 15, 2022