Skip to content
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

Add a directives for general purpose documentation in downstream proj… #57

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

swrichards
Copy link
Contributor

@swrichards swrichards commented Jan 27, 2025

@swrichards swrichards force-pushed the general-usage-sphinx-directives branch 12 times, most recently from d01c07e to e52e817 Compare January 28, 2025 15:19
@swrichards swrichards marked this pull request as ready for review January 28, 2025 15:36
@swrichards
Copy link
Contributor Author

@stevenbal @Coperh the tests don't yet pass but mainly because I mocking out a function in CI is rather tricky due to the convoluted import sequence, trying to fix that now but it won't change the core of the PR, so I am tagging you now for feedback.

@swrichards swrichards force-pushed the general-usage-sphinx-directives branch 3 times, most recently from bb37337 to ab74c09 Compare January 28, 2025 16:07
@Coperh
Copy link

Coperh commented Jan 29, 2025

One thing is that I don't think it includes the templates in the release or at least when I install from github, they are missing. I do get the usage directive though

@swrichards
Copy link
Contributor Author

One thing is that I don't think it includes the templates in the release or at least when I install from github, they are missing. I do get the usage directive though

This is a good point. I was using it locally as an editable install which did copy over the whole thing, but for setuptools it'll probably require a nudge to include the templates in the package. (In the meantime, it is testable with pip install -e, or should be)

@swrichards swrichards force-pushed the general-usage-sphinx-directives branch 6 times, most recently from 40f9e8f to b6a7ec9 Compare January 29, 2025 12:11
Copy link

@Coperh Coperh left a comment

Choose a reason for hiding this comment

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

I think generally it looks good.

I would like the ability to change the template so I can add custom intro paragraph and remove the autoclass since that is not useful for CI/CD users

Comment on lines 17 to 18
_usage_template = Template((_TEMPLATES_PATH / "config_doc.rst").read_text())
_step_template = Template((_TEMPLATES_PATH / "config_step.rst").read_text())
Copy link

Choose a reason for hiding this comment

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

Would be nice to override these templates. Not sure if I can now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Coperh I see a few options:

  1. As you say, making the templates overridable. This has the drawback that Sphinx is really quite picky in building up the RST nodes (e.g. if you were to add headings or other directives, it might fail).
  2. Add some arguments to this directive, for instance: to include/exclude the usage info, and to add a "style" enum for the headings (which could either be the verbose_title, the autodac tag to the step class as now, or a combination of both
  3. Add some additional directives which adds more composability: so the usage text, TOC, and step sections are each their own directives which could be used separately to mix-and-match, with this directive being effectively a bundle of these separate directives.

I am inclined to do that refactoring in a subsequent PR, unless there's an obvious path to 1. we can add here.

docs/config_docs.rst Outdated Show resolved Hide resolved
@Coperh
Copy link

Coperh commented Jan 29, 2025

OR perhaps I could just override the _TEMPLATES_PATH?

Comment on lines 55 to 63
for step_path in configured_steps:
step_cls = import_string(step_path)
step_info = StepInfo(
title=step_cls.verbose_name,
anchor_id=slugify(step_cls.verbose_name),
module_path=step_path,
step_cls=step_cls,
)
steps_info.append(step_info)
Copy link

Choose a reason for hiding this comment

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

I wonder if you can render the step template and add it here?

Copy link

Choose a reason for hiding this comment

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

This node thing is confusing TBH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't get this to work with just inserting the RST output directly. Because you don't know where the directive will be inserted into the final tree, it seems you have to go down this route to explicitly say you want a section with a certain title and so forth (if you do that in RST, the parser will end up complaining about "unexpected title sections" and such things).

@swrichards swrichards force-pushed the general-usage-sphinx-directives branch 4 times, most recently from dd90b6b to d49dd6c Compare January 30, 2025 09:51
@swrichards swrichards marked this pull request as draft January 30, 2025 11:56
@swrichards swrichards force-pushed the general-usage-sphinx-directives branch 2 times, most recently from 2b8f56a to 238f5e2 Compare January 30, 2025 15:23
@swrichards swrichards force-pushed the general-usage-sphinx-directives branch from 238f5e2 to 720f23e Compare January 30, 2025 15:36
@swrichards swrichards marked this pull request as ready for review January 30, 2025 15:39
@swrichards
Copy link
Contributor Author

@stevenbal @Coperh I have added a few options to customize what sections are included/excluded (See the docs and test examples).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants