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

Ignore leading/trailing space in function/method/variable names when linking (closes #80) #148

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jimwins
Copy link
Member

@jimwins jimwins commented Aug 31, 2024

This doesn't quite work, I think the XML in my test case isn't right because the class name isn't actually get linked and I can see that it's not getting indexed correctly.

@jimwins jimwins self-assigned this Aug 31, 2024
@haszi
Copy link
Contributor

haszi commented Aug 31, 2024

@jimwins The top level xml element needs a DocBook xml namespace declaration (ie. xmlns="http://docbook.org/ns/docbook") for PhD to pick up the role attribute correctly.

@haszi
Copy link
Contributor

haszi commented Aug 31, 2024

@Girgias We've talked about this when I wanted to fix this issue and IIRC you preferred fixing the markup in the doc repos instead of silently trimming them.

@alfsb
Copy link
Member

alfsb commented Aug 31, 2024

If it is preferable to fix the sources instead silently trimming, then this code can be changed to verbosely generate alerts where some $target != trim( $target ); on rendering process, to make this type of linking fail easier to catch.

And/or re-implement these types of tests on doc-base scripts, as configure.php or QA tools does not generate alerts of this.

@alfsb
Copy link
Member

alfsb commented Aug 31, 2024

Or to not trim silently, but trim and generate warnings.

@jimwins
Copy link
Member Author

jimwins commented Aug 31, 2024

Adding the namespace declaration doesn't fix the test, it's still not indexing the name of the class for some reason. How that is supposed to get pulled from the contents is still dark magic to me.

Making this class of problem in the XML fail when building instead of just handling it is probably the better solution. Warnings will eventually just get ignored and start piling up.

@alfsb
Copy link
Member

alfsb commented Aug 31, 2024

Making this class of problem in the XML fail when building instead of just handling it is probably the better solution. Warnings will eventually just get ignored and start piling up.

I think that adding warnings in PHD is still good, as linking fails may occurs for other reasons besides whitespace.

About warning/failing this in XML build/configure, I have a old code from when doc-base/scripts/translation was still a remote dream, that outputs this:

qaxml.w: en/reference/outcontrol/user-level-output-buffers.xml

  methodname handler

qaxml.w: en/reference/reflection/reflectionreference/fromarrayelement.xml

  classname ReflectionReference

@haszi
Copy link
Contributor

haszi commented Aug 31, 2024

Adding the namespace declaration doesn't fix the test, it's still not indexing the name of the class for some reason. How that is supposed to get pulled from the contents is still dark magic to me.

Try adding <titleabbrev>Example</titleabbrev> before the <classsynopsis> element. This might cause an issue with whitespace but should fix the indexing issue.

@jimwins
Copy link
Member Author

jimwins commented Aug 31, 2024

Okay, found the problem with my test, and it's because the way the anchors are captured for <classname> in phpdotnet/phd/Package/PHP/XHTML.php based on the rules at line 151. (It uses the <titleabbrev>, not anything to do with the <classsynopsis> like I had expected.)

We could print warnings about missing links in the get*Link() methods in phpdotnet/phd/Format.php. That prints 6,687 warnings for doc-en right now. I haven't tried to do any sort of breakdown, but I think a substantial fraction of them are probably expected (like references to old functions in migration chapters, or un-namespaced classes within the documentation for extensions).

@haszi
Copy link
Contributor

haszi commented Aug 31, 2024

Making this class of problem in the XML fail when building instead of just handling it is probably the better solution. Warnings will eventually just get ignored and start piling up.

I think that adding warnings in PHD is still good, as linking fails may occurs for other reasons besides whitespace.

I agree with warning/failing in doc-base/configure.php but disagree with doing this in PhD as there are a lot of non-linking deprecated and removed constants and functions in the migration guides which would generate hundreds or more warnings on every run of PhD.

@Girgias
Copy link
Member

Girgias commented Sep 11, 2024

@Girgias We've talked about this when I wanted to fix this issue and IIRC you preferred fixing the markup in the doc repos instead of silently trimming them.

I didn't have much preference but @salathe IIRC preferred fixing the XML sources.

I'm okay with allowing the linking, but it should warn so that the sources can be fixed.

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