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

Introduce phpdoc guides builder #594

Merged
merged 23 commits into from
Sep 21, 2024
Merged

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Jul 12, 2024

I introduced an interface to be able to switch between the different implementations. The first setup of guides is done at the required locations and allows us to render the documentation. However this doesn't work yet without issues. More might be needed consider this work in progress.

  • introduce new code highlighter
  • Check why tocs do not contain all items like before
  • Add support for toc and tocheader directives
  • Add template for toc
  • Review existing directives, and add new ones for the ones that are not supportes by guides.
  • check what is wrong with the code highlighter in orm
  • Recreate search index using new nodes

@jaapio
Copy link
Contributor Author

jaapio commented Aug 6, 2024

I started adding tasks for the things I want to do in this PR, will add more when I find issues.

@jaapio jaapio marked this pull request as ready for review August 20, 2024 19:57
@jaapio
Copy link
Contributor Author

jaapio commented Aug 20, 2024

@SenseException could you please give me some direction on how to continue with this. I do see that tests are needed and some static analysis issues must be resolved. My first intention was to keep the old doc rendering working if needed I can restore that but I needed to remove some logic in the copy actions because I replaced the way the sidebar menu is moved.

I'm sure this change will break the search as I didn't address that yet and we could also remove all old rst-parser related code to make this a fresh restart. But I'm not sure we should cover all that in this PR.

Please let me know what you think.

lib/Docs/RST/RSTCopier.php Outdated Show resolved Hide resolved
lib/Docs/RST/RSTPostBuildProcessor.php Outdated Show resolved Hide resolved
templates/guides/body/admonition.html.twig Show resolved Hide resolved
config/packages/guides.yml Outdated Show resolved Hide resolved
lib/Docs/RST/Logger.php Show resolved Hide resolved
lib/Docs/RST/DoctrineRSTBuilder.php Outdated Show resolved Hide resolved
config/config.yml Outdated Show resolved Hide resolved
@SenseException
Copy link
Member

I've left some comments in the code rather doing a first review. Some (probably now outdated) tests are failing and the static analysis issues need to be addressed.

Copy link
Contributor Author

@jaapio jaapio left a comment

Choose a reason for hiding this comment

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

Some responses added

@greg0ire
Copy link
Member

Suggestion: target staging instead of master. That way, if you'd rather work with several PRs, we can do that without impacting the real website, and we won't block other PRs on master while doing so.

@jaapio jaapio force-pushed the phpdoc-guides branch 2 times, most recently from 3e4e636 to 9a1297d Compare August 21, 2024 19:17
@jaapio jaapio changed the base branch from master to staging August 21, 2024 19:19
jaapio added 10 commits August 21, 2024 22:48
I introduced an interface to be able to switch between the different
implementations. The first setup of guides is done at the required locations and allows us to render the documentation. However this doesn't work yet without issues. More might be needed consider this work in progress.
@jaapio jaapio force-pushed the phpdoc-guides branch 3 times, most recently from 9b7c5c1 to a350f85 Compare September 17, 2024 20:13
@jaapio
Copy link
Contributor Author

jaapio commented Sep 18, 2024

Ok guys, this seems to be green and passing. I'm really looking forward to your experience and review. Let me know if there are any questions!

@greg0ire
Copy link
Member

Very few comments on my end, this looks good. What are the next steps? We merge this, then review the staging website, and work on fixing whatever is broken?

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Very few comments on my end, this looks good. What are the next steps? We merge this, then review the staging website, and work on fixing whatever is broken?

stage isn't part of a process, but more a trigger and test-branch if you want to share changes that more people are supposed to look into. Since stage became a target branch for this PR, after all comments were addressed, it would be merged to stage and we would test the documentation and "fix whatever is broken" as you already mentioned it.

AFAIK is the search indexer the only thing we currently cannot test on stage.

lib/Docs/RST/RSTCopier.php Outdated Show resolved Hide resolved
lib/Guides/StaticWebsiteGenerator/GuidesRstConverter.php Outdated Show resolved Hide resolved
@@ -0,0 +1,29 @@
---
title: "{{ node.pageTitle | yaml_encode }}"
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this yaml encoding?

phpstan.neon.dist Show resolved Hide resolved
title: "{{ node.pageTitle | yaml_encode }}"
docsIndex: {% if node.filePath ends with 'index' %}true{% else %}false{% endif %}

docsSourcePath: "/en/{{ node.filePath }}.rst"
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be tested for old Doctrine 1 once we have this in the stage branch.

{% endif %}
{%~ endfor -%}

{{ "{% endverbatim %}" }}
Copy link
Member

Choose a reason for hiding this comment

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

What is this output for?

tests/Twig/MainExtensionTest.php Outdated Show resolved Hide resolved
@jaapio
Copy link
Contributor Author

jaapio commented Sep 19, 2024

To answer some of the questions asked in general. We need to have a look at the result of the docs parser, which are twig templates. That's why you see yaml and twig tags in the output and templates. Those templates are picked up by the website build command and processed into the actual page.

This wasn't my idea but how it currently works. Given the fact that the docs templates do not have information about the other projects and pages I do understand this approach. But it results into very interesting template code.
The yaml for example is used in the search index building. And some other locations. We can improve this, but I think that's out of scope for now.

As toctree does modify the structure of the docs, which results
in infinitif loops when items refer to eachother, we switched
to menu.
To allow full usage of the configuration move the theme configuration
to a compiler pass that will set the value.
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Great work 👍

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Let's see how this looks on staging 👍

@SenseException SenseException merged commit 557eae8 into doctrine:staging Sep 21, 2024
5 checks passed
@jaapio jaapio deleted the phpdoc-guides branch September 21, 2024 20:51
@jaapio
Copy link
Contributor Author

jaapio commented Sep 21, 2024

Thanks for your reviews!

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.

3 participants