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 DOCtor-RST #613

Merged
merged 14 commits into from
Feb 26, 2020
Merged

Introduce DOCtor-RST #613

merged 14 commits into from
Feb 26, 2020

Conversation

OskarStark
Copy link
Contributor

@OskarStark OskarStark commented Jan 31, 2020

DOCtor-RST is a linting tool (which runs as GithubAction), which is used at symfony-docs, Sonata and EasyAdminBundle.

It checks the rst files for some specific conventions.

You can see it in Action here: https://github.com/symfony/symfony-docs/pull/13006/checks?check_run_id=414955239

We are using it since a few months for the symfony-docs and is developed by me 😄

https://github.com/OskarStark/doctor-rst

@OskarStark OskarStark changed the base branch from master to 3.6.x January 31, 2020 15:02
@alcaeus
Copy link
Member

alcaeus commented Jan 31, 2020

@OskarStark could you please add a comment to tell me what DOCtor-RST does? ;)

@OskarStark
Copy link
Contributor Author

OskarStark commented Jan 31, 2020

Sure, I updated the PR header 👍

PS: Best reviewed commit-by-commit

@@ -343,17 +343,13 @@ following syntax:
</doctrine_mongodb:config>
</container>

Now you can retrieve the configured services connection services:

.. code-block:: php
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a bad thing?

Copy link
Contributor Author

@OskarStark OskarStark Feb 1, 2020

Choose a reason for hiding this comment

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

It’s not called bad, but more noise then you need and it reads more fluently.
Also you have highlighted code block in PHPStorm for example

Copy link
Member

@malarzm malarzm Feb 1, 2020

Choose a reason for hiding this comment

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

Ah ok, I thought it was the php part that would allow IDEs to understand there's PHP (not brainfuck for one) inside the code block (or tell renderer later it should use PHP rules for code highlighting)

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

This looks great, I'd love to have it run on ODM's docs as well 🎉

@OskarStark
Copy link
Contributor Author

Thank you 😊 my plan is to add it to all doctrine repositories 😊✌🏻

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM. @SenseException can we test this on the website before rolling it out? I would like to confirm that the changes to code blocks don't have unwanted side effects.

@SenseException
Copy link
Member

The website only reads the docs of repository branches from the .doctrine-project.json file on master. Maybe I can test it locally in this case.

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.

I get this error during the docs-build: /!\ Error while processing "code-block" directive in "form": Unknown language: html+twig

@OskarStark
Copy link
Contributor Author

Hi @SenseException can you tell me which Sphinx version you are using or can I see your setup?

We at Sonata-Project and Symfony (docs) added a GithubActions workflow to build the docs and see if its working. Are you interested in this too? If yes I could provide one in a separate PR 👍

@alcaeus
Copy link
Member

alcaeus commented Feb 11, 2020

We at Sonata-Project and Symfony (docs) added a GithubActions workflow to build the docs and see if its working. Are you interested in this too?

Yes please. 😍

@SenseException
Copy link
Member

The Doctrine website is currently using https://github.com/doctrine/rst-parser for creating the docs, but having Sphinx would be great too. It can create a HTML documentation pretty fast for a big amount of ReStructuredText files.

@OskarStark
Copy link
Contributor Author

Hmm I have no experience with the rst parser. Does my PR works after dropping this commit? ec846d5

Can you do and try or shall I drop the commit from this PR?

Or maybe just tell the parser to render twig the same as jinja? 🤔

@SenseException
Copy link
Member

For now please use html+jinja again. Did DOCtor marked this as an issue?

I wrote a small example from the parser and pushed it into https://github.com/SenseException/rst-builder. I think it misses a few implementations that got directly implemented into https://github.com/doctrine/doctrine-website itself where the documentation of every version is built. If Sphinx will at one point of time be chosen as a new rst-builder, it would be needed in the build steps of the Doctrine website.

@OskarStark
Copy link
Contributor Author

OskarStark commented Feb 11, 2020 via email

@OskarStark
Copy link
Contributor Author

Done ✅ can you please check again if the rst-parser is complaining about sth? Thanks 🙏

@SenseException
Copy link
Member

SenseException commented Feb 12, 2020

It's looking good so far. The code blocks where you replaced .. code-block:: php with :: will result in missing highlighting and missing line numbers.

This is how the docs currently look like:
docs-orig

And this how it looks like with your changes:
docs-new

@OskarStark
Copy link
Contributor Author

Hi @SenseException,

thank you for your feedback. I cannot say much about the RST-Parser of doctrine and we had the same "problem" at the symfony-docs repo, to not have highlighting when using ::. We fixed it by telling sphinx "Hey we do mostly PHP stuff, so use it by default" in our config

We can ofc. keep it like before and deactivate the rule or switch to Sphinx, but I don't know which advantages doctrine/rst-parser has at the moment

@malarzm
Copy link
Member

malarzm commented Feb 25, 2020

@OskarStark could you take a look at #613 (comment) ? Do we need to use a default language setting somewhere so the code block won't lose their formatting?

@OskarStark
Copy link
Contributor Author

I just answered 1 minute before 😄

@malarzm
Copy link
Member

malarzm commented Feb 25, 2020

Hah perfect timing :) IIRC RST-Parser was introduced to get rid of sphinx as we were moving out from readthedocs. If we could keep the language setting per code block and it won't bother Symfony ecosystem that'd be great.

@OskarStark
Copy link
Contributor Author

If we could keep the language setting per code block and it won't bother Symfony ecosystem that'd be great.

Sure thing, it will not bother us, its just (because we have a lot of docs), less distracting/noisy to read, I will disable the rule and revert the commit

Comment on lines +37 to +47
versionadded_directive_major_version:
major_version: 3

versionadded_directive_min_version:
min_version: '3.0'

deprecated_directive_major_version:
major_version: 3

deprecated_directive_min_version:
min_version: '3.0'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After up merging into 4.x this must be change accordingly. Same for master, if this will be next 5.0, otherwise not.

@OskarStark
Copy link
Contributor Author

I will disable the rule and revert the commit

done

@OskarStark
Copy link
Contributor Author

For the record we use this extension for symfony-docs Sphinx:
https://github.com/fabpot/sphinx-php#sphinx-extensions-for-php-and-symfony

@malarzm
Copy link
Member

malarzm commented Feb 26, 2020

Let's get this merged, thanks a lot @OskarStark!

@malarzm malarzm merged commit 5dd3f0d into doctrine:3.6.x Feb 26, 2020
@OskarStark
Copy link
Contributor Author

Wohoo 🙌 thanks 🥳

@SenseException
Copy link
Member

Thank you 🌷

@OskarStark OskarStark deleted the doctor-rst branch February 27, 2020 07:02
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.

4 participants