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

Ref role resolving #233

Merged
merged 7 commits into from
Mar 14, 2023
Merged

Ref role resolving #233

merged 7 commits into from
Mar 14, 2023

Conversation

jaapio
Copy link
Member

@jaapio jaapio commented Mar 7, 2023

The text role ref is now supported as it should.
Simplyfied the way links are rendered without markup in the titles.
This allows us again to create links using just text.

There is a major issue with using the subparsers in this change
the sub documents are isolated from the main document. This causes
issues handling includes. We should refactor this in another effort.

@jaapio
Copy link
Member Author

jaapio commented Mar 7, 2023

@wouterj this supersedes the PR you created #21 and should fixes #16

@jaapio
Copy link
Member Author

jaapio commented Mar 7, 2023

@linawolf I would like to have your opinion on the subparser. I think we do not need it anymore as we have a different way of doing this using the \phpDocumentor\Guides\RestructuredText\Parser\Productions\RuleContainer you can see an example of this in \phpDocumentor\Guides\RestructuredText\Parser\Productions\SimpleTableRule.

Right now Admonitions might contain everything, but I think in the should not have all nodes and directives. Some directives and nodes are not allowed to be used in directives. following https://docutils.sourceforge.io/docs/ref/doctree.html there are categories of elements that I tried to model using the parser setup.

And for example, the includes do not work as they should, as we should literally include the nodes of the included file. So title levels are kept the same when a document is included.

@wouterj
Copy link
Contributor

wouterj commented Mar 7, 2023

Thanks for continuing my work! I'll likely try these changes out in the Symfony docs and do a review on Thursday

@linawolf
Copy link
Contributor

linawolf commented Mar 8, 2023

I am honestly still getting used to the new architecture by fixing some tiny things. Not far enough in to have an oppinion, really

@wouterj
Copy link
Contributor

wouterj commented Mar 14, 2023

Not sure if it's 100% related to this one, but when rendering the Symfony docs on this branch (rebased on #252) there are some invalidly detected references.

Examples:

``choice``: renders one, two (default) or three select inputs (hour, minute,
everything after : is detected as a single reference (there is no reference on the line).

When using different values for `model_timezone`_ and ``view_timezone``,
this whole line is detected as a single reference (instead of only model_timezone)

**type**: ``integer`` **default**: Locale-specific (usually around ``3``)
Locale-specific (usually around ``3``) is detected as reference

Securing by an Expression
gives an error "invalid cross reference: ecuring by an Expression"


Other than this, it seems to work flawless on the Symfony docs! :)

@jaapio
Copy link
Member Author

jaapio commented Mar 14, 2023

I think this are bugs in the span parser. I need to have a look. But not related to this pr.

wouterj and others added 6 commits March 14, 2023 21:19
Using the ref role, you can create a hyperlink to an explicit internal
link target (anchor nodes).
The text role `ref` is now supported as it should.
Simplyfied the way links are rendered without markup in the titles.
This allows us again to create links using just text.

There is a major issue with using the subparsers in this change
the sub documents are isolated from the main document. This causes
issues handling includes. We should refactor this in another effort.
Internal target is part of the metas, as it is build
from the documents, and overlapping for the full set.
During a refactoring the document links were broken, the alternative
text was ignored.
@jaapio jaapio marked this pull request as ready for review March 14, 2023 20:54
@jaapio jaapio merged commit f47cc85 into main Mar 14, 2023
@jaapio jaapio deleted the feature/references branch March 14, 2023 21:43
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.

None yet

3 participants