-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: parse xrefs differently with new xref format #90
Conversation
Testing on Kokoro with |
docfx_yaml/extension.py
Outdated
initial_index += summary_part.find("<xref") | ||
original_type = parsed_text[initial_index:initial_index+(parsed_text[initial_index:].find('>'))+1] | ||
original_type = parsed_text[initial_index:initial_index+(parsed_text[initial_index:].find('/xref>'))+6] |
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.
What is 6? Can we make that a constant or something?
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.
Done.
initial_index += len(original_type) | ||
original_type = " ".join(filter(None, re.split(r'\n| |\|\s|\t', original_type))) | ||
safe_type = 'xref_' + original_type[6:-1] | ||
# Extract text from "<xref uid="uid">text</xref>" |
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.
It's a bit late now, but we could also add this UID to the references
section with however we want it to show up. Then, we wouldn't need to do any HTML parsing.
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.
I'll keep that in mind perhaps for non-Cloud or third party xrefs.
if arg_name not in summary_info[var_types[cur_type]] and ':raises' not in cur_type: | ||
summary_info[var_types[cur_type]][arg_name] = {} | ||
except KeyError: | ||
raise KeyError(f"Encountered wrong formatting, please check docstring for {name}") |
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.
I'm not sure this will work. What if there is a release with bad docs? We still need to generate docs for it. So, we should make our best guess and move on.
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.
Repos should have a docs-presubmit
check for PRs, which runs the docs job to see whether it successfully runs this plugin on the repo for sphinx-build
or not. For example see googleapis/python-firestore#407
While it's usually not set to be a required check, this lets contributors know that docs job would fail or pass before triggering a release.
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.
Sounds good. Might need to update this depending on how back filling old libraries goes.
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.
I do have the Kokoro job set up that could try and run it for all versions, I'll try testing on that for older versions.
if arg_name not in summary_info[var_types[cur_type]] and ':raises' not in cur_type: | ||
summary_info[var_types[cur_type]][arg_name] = {} | ||
except KeyError: | ||
raise KeyError(f"Encountered wrong formatting, please check docstring for {name}") |
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.
Sounds good. Might need to update this depending on how back filling old libraries goes.
Before you open a pull request, note that this repository is forked from here.
Unless the issue you're trying to solve is unique to this specific repository,
please file an issue and/or send changes upstream to the original as well.
Updating xref parser to accommodate the new and only xref format:
<xref uid="uid">text</xref>
.We need to be careful on two parts:
text
portion to replace so GoogleDocstring doesn't complain on the leftover xref syntaxFixes #89
Fixes googleapis/python-firestore#408