fix: add hardcoded IAM references temporarily #209

dandhlee · 2022-05-17T05:41:17Z

At the moment, there are 4 IAM libraries and it is very unclear to users which IAM library is referenced in the documentation. To make this worse, some of them don't have documentation that is generated to point to. The IAM team is reviewing combining all the libraries under python-iam, but until then it will remain unclear.

To temporarily address this, pointing the IAM references to the GitHub repository where the source code is located. This hard coded reference list can also be used for other common libraries that the documentation is missing references to, until we can figure out Intersphinx and hosting that on c.g.c.

Filed #208 to track removing the hard coded references in the future.

Fixes b/232773389.

Tests pass

docfx_yaml/extension.py

tbpg · 2022-05-17T12:25:55Z

docfx_yaml/extension.py

    # Using counter to check if the entry is already a cross reference.
    for index, word in enumerate(words):
        cross_reference = ""
        for keyword in entry_names:
            if keyword != current_name and keyword not in current_name and keyword in word:
+                if keyword in hard_coded_references:
+                    if "<a" in words[index-1] or \


The indentation here is pretty gnarly (for -> for -> if -> if -> if with a couple continues). Can we simplify this? Perhaps with a helper function?

Done, updated to use a helper function.

docfx_yaml/extension.py

Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>

dandhlee · 2022-05-17T18:53:57Z

Thank you! Please take a look again.

docfx_yaml/extension.py

tbpg · 2022-05-17T17:36:17Z

docfx_yaml/extension.py

 def convert_cross_references(content: str, current_name: str, entry_names: List[str]):
+    """Finds and replaces references that should be a cross reference in given content.
+
+    This should not convert any references that contain its name, i.e. if we're processing


Does "its name" refer to current_name?

Yes. Updated comment to clarify this.

tbpg · 2022-05-17T18:42:17Z

docfx_yaml/extension.py

+    converted_words: List[str],
+    hard_coded_references: Dict[str, str] = None
+):
+    """Given a word, check if it contains a reference keyword we should convert.


"Check" sounds like a boolean. So, I'd suggest something like resolve_cross_reference and perhaps return None if nothing is found instead of "".

I felt icky mixing between None and an empty string, however I think in this case it makes sense that we indeed return None instead of an empty string if a cross reference has not been found. Updating with suggestions.

👍 Please update this docstring. Something like Returns the resolved cross reference for current_word, if it exists.

docfx_yaml/extension.py

tbpg · 2022-05-17T18:42:59Z

docfx_yaml/extension.py

+    current_name: str,
+    words: List[str],
+    index: int,
+    word: str,


The word and current_name args are confusing to me.

Updated word -> current_word and current_name -> current_object_name. However, checking for it in this function seemed useless so I removed it from this function and moved it up, happy to take any other variable name suggestions still.

tbpg · 2022-05-17T18:46:10Z

docfx_yaml/extension.py

+        if keyword in hard_coded_references:
+            if "<a" not in words[index-1] and \
+                not (converted_words and f"<a href=\"{hard_coded_references[keyword]}" in converted_words[-1]):
+                cross_reference = f"<a href=\"{hard_coded_references[keyword]}\">{keyword}</a>"


Could just return at this point.

I thought it was better style to put the return at the end, but I have no preference. Updated to reflect suggestion.

docfx_yaml/extension.py

tbpg · 2022-05-17T19:00:54Z

docfx_yaml/extension.py

    words = content.split(" ")
-    new_words = []
+    converted_words = []


Why a list instead of a dictionary from word to converted word?

I suppose the name is still throwing off. As we traverse the original list of words, we still need to check against the converted list of words and order matters. At the end, I use this to simply convert to a string using a join. Will update the name so it's not misleading.

dandhlee · 2022-05-18T05:50:08Z

Thank you! Made necessary modifications, updated existing unit test for checking against the name itself and name change in the test. Please take a look again :)

tbpg

Sorry for all the comments -- trying to wrap my head around this. 😄

docfx_yaml/extension.py

tbpg · 2022-05-18T12:10:47Z

docfx_yaml/extension.py

-                cross_reference = f"<xref uid=\"{keyword}\">{keyword}</xref>"
-                new_words.append(word.replace(keyword, cross_reference))
+            # Do not convert references to itself.
+            if keyword in current_object_name:


Apologies if this was asked before. Why is this in and not ==? I'm imagining something like foo.Foo and foo.FooBar. I suppose there might be other punctuation or something?

When we check for foo, we don't check to convert references for foo.* yet, because the parents (i.e. foo is the parent of foo.Foo) come before its children and would try to convert foo.Foo as <xref uid='foo'>foo</xref>.Foo instead of <xref uid='foo.Foo'>foo.Foo</xref>. On the contrary, foo.Foo in foo doesn't hold and will properly be handled later, as shown here for google.cloud.bigquery_storage_v1.types.ReadSession.TableReadOptions. Totally my fault for not explaining clearly, I will add a comment to clarify!

It looks like we split on " "? So, do we need to worry about foo.Foo being split up?

Nay, they will always appear in tact as one word since split(" ") needs the words to have whitespace in between. If it does, that's a docstring typo rather than logical overhead from this code.

tbpg · 2022-05-18T12:12:24Z

docfx_yaml/extension.py

+
+    # Contains a list of words that is not a valid reference or converted
+    # references.
+    visited_words = []


What about processed, resolved, or converted content? Or even result?

(Visited makes me think of doing a graph traversal.)

Processed sounds good!

docfx_yaml/extension.py

tbpg · 2022-05-18T12:18:42Z

docfx_yaml/extension.py

+    converted_words: List[str],
+    hard_coded_references: Dict[str, str] = None
+):
+    """Given a word, check if it contains a reference keyword we should convert.


👍 Please update this docstring. Something like Returns the resolved cross reference for current_word, if it exists.

tbpg · 2022-05-18T12:19:01Z

docfx_yaml/extension.py

+    Args:
+        words: list of words used to check and compare content before and after `current_word`
+        index: index position of `current_word` within words
+        current_word: current word being looked at


Total nit: perhaps current_word should be the first arg?

docfx_yaml/extension.py

Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>

dandhlee

No worries, thank you for taking a detailed look! :D

dandhlee · 2022-05-18T12:49:23Z

docfx_yaml/extension.py

-                cross_reference = f"<xref uid=\"{keyword}\">{keyword}</xref>"
-                new_words.append(word.replace(keyword, cross_reference))
+            # Do not convert references to itself.
+            if keyword in current_object_name:


When we check for foo, we don't check to convert references for foo.* yet, because the parents (i.e. foo is the parent of foo.Foo) come before its children and would try to convert foo.Foo as <xref uid='foo'>foo</xref>.Foo instead of <xref uid='foo.Foo'>foo.Foo</xref>. On the contrary, foo.Foo in foo doesn't hold and will properly be handled later, as shown here for google.cloud.bigquery_storage_v1.types.ReadSession.TableReadOptions. Totally my fault for not explaining clearly, I will add a comment to clarify!

dandhlee · 2022-05-18T12:54:22Z

docfx_yaml/extension.py

+
+    # Contains a list of words that is not a valid reference or converted
+    # references.
+    visited_words = []


Processed sounds good!

docfx_yaml/extension.py

dandhlee · 2022-05-18T13:14:05Z

docfx_yaml/extension.py

+    Args:
+        words: list of words used to check and compare content before and after `current_word`
+        index: index position of `current_word` within words
+        current_word: current word being looked at


dandhlee · 2022-05-18T13:22:04Z

docfx_yaml/extension.py

+    converted_words: List[str],
+    hard_coded_references: Dict[str, str] = None
+):
+    """Given a word, check if it contains a reference keyword we should convert.


tbpg · 2022-05-18T13:50:15Z

docfx_yaml/extension.py

-                cross_reference = f"<xref uid=\"{keyword}\">{keyword}</xref>"
-                new_words.append(word.replace(keyword, cross_reference))
+            # Do not convert references to itself.
+            if keyword in current_object_name:


It looks like we split on " "? So, do we need to worry about foo.Foo being split up?

tbpg · 2022-05-18T13:51:00Z

docfx_yaml/extension.py

+        current_word: current word being looked at
+        words: list of words used to check and compare content before and after `current_word`
+        index: index position of `current_word` within words
+        keyword: reference keyword to check against


Last arg question - what exactly is keyword?

Doesn't have to be last! I'm not sure why I stuck with that word in this context, they're UIDs extracted from Sphinx. Updated to say UIDs instead.

made additional modifications to the PR

dandhlee

Made some additional modifications, please take a look again!

dandhlee · 2022-05-18T14:38:07Z

docfx_yaml/extension.py

-                cross_reference = f"<xref uid=\"{keyword}\">{keyword}</xref>"
-                new_words.append(word.replace(keyword, cross_reference))
+            # Do not convert references to itself.
+            if keyword in current_object_name:


Nay, they will always appear in tact as one word since split(" ") needs the words to have whitespace in between. If it does, that's a docstring typo rather than logical overhead from this code.

dandhlee · 2022-05-18T14:42:30Z

docfx_yaml/extension.py

+        current_word: current word being looked at
+        words: list of words used to check and compare content before and after `current_word`
+        index: index position of `current_word` within words
+        keyword: reference keyword to check against


Doesn't have to be last! I'm not sure why I stuck with that word in this context, they're UIDs extracted from Sphinx. Updated to say UIDs instead.

dandhlee added 2 commits May 17, 2022 05:36

fix: add hardcoded reference for IAM references

0276831

test: update unit test

11a57a6

dandhlee requested a review from a team as a code owner May 17, 2022 05:41

dandhlee requested a review from a team May 17, 2022 05:41

product-auto-label bot added the size: s Pull request size is small. label May 17, 2022

tbpg requested changes May 17, 2022

View reviewed changes

dandhlee and others added 2 commits May 17, 2022 11:37

fix: apply suggestion

27e22b0

Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>

fix: separate code into helper function and add proper docstrings

c318ce0

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels May 17, 2022

dandhlee added 2 commits May 17, 2022 18:44

fix: ensure hard_coded_references is optional and not used if not found

d7b0276

test: update unit test with new helper function

f858db1

dandhlee requested a review from tbpg May 17, 2022 18:54

tbpg requested changes May 17, 2022

View reviewed changes

dandhlee added 2 commits May 18, 2022 05:47

fix: update method name and apply review suggestions

7d5e1ce

test: update unit test

cd911e2

dandhlee requested a review from tbpg May 18, 2022 05:50

tbpg requested changes May 18, 2022

View reviewed changes

dandhlee and others added 2 commits May 18, 2022 08:31

fix: address review suggestion for adding return type

abca6ae

Co-authored-by: Tyler Bui-Palsulich <26876514+tbpg@users.noreply.github.com>

fix: address review comments

73d76a7

dandhlee commented May 18, 2022

View reviewed changes

dandhlee requested a review from tbpg May 18, 2022 13:23

tbpg previously approved these changes May 18, 2022

View reviewed changes

dandhlee added 3 commits May 18, 2022 14:44

fix: update variable name from keyword to uid

7b4db94

fix: update method names, variable names and refactor methods

21af775

test: update unit test

dd5e318

dandhlee requested a review from tbpg May 20, 2022 22:36

fix: update comments, move hardcoded bits to the back

54bc9e0

dandhlee commented May 23, 2022

View reviewed changes

Merge branch 'main' into fix_iam

7623f7f

tbpg approved these changes May 24, 2022

View reviewed changes

dandhlee merged commit 5dc99d2 into main May 24, 2022

dandhlee deleted the fix_iam branch May 24, 2022 18:47

release-please bot mentioned this pull request May 24, 2022

chore(main): release 1.4.8 #211

Merged

fix: add hardcoded IAM references temporarily #209

fix: add hardcoded IAM references temporarily #209

Conversation

dandhlee commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandhlee commented May 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandhlee commented May 18, 2022

tbpg 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment