-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[docs-beta] Add a Sphinx MDX builder #24235
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @PedramNavid and the rest of your teammates on Graphite |
cfddc8e
to
0954726
Compare
rm -rf ../docs-beta/docs/api/*.mdx | ||
cp -rf _build/mdx/index.mdx ../docs-beta/docs/api/ | ||
|
||
# Catch-all target: route all unknown targets to Sphinx using the new |
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.
This is the new Sphinx template taken directly from the sphinx quickstart. It passes through builders instead of listing them all here.
@@ -155,7 +155,7 @@ def check_custom_errors(app: Sphinx, exc: Optional[Exception] = None) -> None: | |||
if len(dagster_errors) > 0: | |||
for error_msg in dagster_errors: | |||
logger.info(error_msg) | |||
raise Exception( | |||
logger.error( |
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.
There were over 100 undocumented public APIs listed when this ran. Not sure why it wasn't being caught when building our JSON docs but they are being caught now, so had to make this log instead of Raise.
0954726
to
1bc50c7
Compare
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-fk6i69zpy-elementl.vercel.app Direct link to changed pages: |
66fbc17
to
24d0b92
Compare
Deploy preview for dagster-docs-beta ready! Preview available at https://dagster-docs-beta-cnm4ktlbw-elementl.vercel.app Direct link to changed pages: |
message = ( | ||
f"Docstring not found for {object.__name__}.{name}. " | ||
f"Docstring not found for {obj.__name__}.{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.
f"Docstring not found for {obj.__name__}.{name}. " | |
f"Docstring not found for {obj.__name__}.{name}. " # type: ignore[reportAttributeAccessIssue] |
Appease the Pyright gods.
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.
should actually be repr(obj) not every obj has a 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.
Really awesome work.
Next time we huddle I would appreciate it if you could walk me through the end_state
function, as I feel it's pretty crucial to this, and I didn't fully grok it.
self.file_transform = self.config.mdx_file_transform or file_transform | ||
self.link_transform = self.config.mdx_link_transform or link_transform | ||
|
||
def get_outdated_docs(self) -> Iterator[str]: |
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.
This is neat, I'm surprised it's a required override and not built-in (referencing this doc).
So only updated source files will be re-rendered?
|
||
logger = logging.getLogger(__name__) | ||
STDINDENT = 4 | ||
MAXWIDTH = 120 |
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.
Would there be benefit in making this configurable?
I see the use of the custom wrapper in end_state()
/ do_format()
, but could you provide an example of when this would occur? For example, when we depart_Title
, if the title was >120 characters, would it go onto a new line?
This adds a Sphinx MDX Builder extension that can generate mdx files from RST. It makes some modifications to our existing Sphinx config that were causing errors. Namely, the dagster extension was raising Exceptions during the build for non-public APIs, however this seems to only run on non-json builds. This is changed to only log the output for now.
type This was a fun one. Essentially, a class __dict__ returns none when `__doc__` is called, eg. with `obj.__doc__`. Instead, you can assert that docs exist with `hasattr`. This was causing PipesContext and other classes to erroneously report no documentation.
4133f71
to
4eaf03e
Compare
4eaf03e
to
506b592
Compare
Merge activity
|
This adds a Sphinx MDX Builder extension to generate MDX files from RST. It made some modifications to our existing Sphinx configuration that were causing errors. Namely, the dagster extension was raising Exceptions during the build for non-public APIs, however this seems to only run on non-json builds. This is changed to only log the output for
now.
Also fixed a bug where methods were being flagged as missing docs when they were not. Essentially, a class dict returns none when doc is called, eg. with obj.doc. Instead, you can assert that docs exist with hasattr. This was causing PipesContext and other classes to report no documentation erroneously.
Summary & Motivation
To populate the new docs site with API docs
How I Tested These Changes
local, bk
This should not affect the existing docs site, will use BK to validate.
Changelog [New | Bug | Docs]
NOCHANGELOG