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

Fix references resolving #60

Merged
merged 24 commits into from
Aug 18, 2023
Merged

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jul 17, 2023

fix part of #56 - fixes absolute references (but declarations still cannot currently contain \)

also fix #58

also missing targets are newly reported, as infos only, as some targets might be in other projects, global PHP classes, ... and CI must pass

also the testing is unified and the html output for all original tests is asserted using CI

also the code is reformatted, in separate commit, using Black to enforce strict coding style

output of all rst test files is exactly the same as before this PR -> no BC break

@mvorisek mvorisek force-pushed the fix_ns_resolving branch 6 times, most recently from d2065d1 to 0e7532a Compare July 31, 2023 22:36
@mvorisek mvorisek changed the title Fix NS resolving Add support for absolute reference names Jul 31, 2023
@mvorisek mvorisek force-pushed the fix_ns_resolving branch 2 times, most recently from 2605f92 to 3731f66 Compare July 31, 2023 22:44
@mvorisek mvorisek changed the title Add support for absolute reference names Fix resolve for absolute reference names Jul 31, 2023
@mvorisek mvorisek marked this pull request as ready for review July 31, 2023 22:48
@mvorisek mvorisek mentioned this pull request Aug 1, 2023
@mvorisek mvorisek marked this pull request as draft August 1, 2023 09:19
@mvorisek mvorisek force-pushed the fix_ns_resolving branch 2 times, most recently from 3e8ad0d to c5e199d Compare August 1, 2023 09:29
@mvorisek mvorisek marked this pull request as ready for review August 1, 2023 09:33
@mvorisek mvorisek force-pushed the fix_ns_resolving branch 3 times, most recently from 4f3548b to 355bfa9 Compare August 1, 2023 11:14
@mvorisek mvorisek changed the title Fix resolve for absolute reference names Fix references resolving Aug 2, 2023
@mvorisek mvorisek force-pushed the fix_ns_resolving branch 5 times, most recently from 03b2eec to b002ba8 Compare August 3, 2023 10:54
@mvorisek mvorisek force-pushed the fix_ns_resolving branch 3 times, most recently from a867191 to 7c32164 Compare August 3, 2023 16:17
@mvorisek
Copy link
Contributor Author

mvorisek commented Aug 3, 2023

@markstory sorry for a lot of noise :), but I had to fix a lot of small issues in order to pass all the added tests. This PR is finally done, more to come in other PRs.

@markstory
Copy link
Owner

This is a big change, it might take me a few days to get through it.

@@ -1,3 +1,4 @@
```{eval-rst}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this to simplify build configuration? Having this markdown/rst hybrid format be the default isn't ideal to me. It makes a less-frequently used package a hard dependency and requirement for long term maintenance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sphinx requires only one file format. To allow to test everything at once, we need md files + {eval-rst}.

  • MyST parser needs to be tested to assert the parsing is done solely by Sphinx and the phpdomain does not operate on raw rst. This is related with Native directive params support #55 (I plan to fix it in another PR).

  • {eval-rst} seems to work perfectly as native rst, Sphinx simply does not read rst file directly, but instead the wrapped string. I did not experience any difference between native rst file and rst from evaluated string.

MyST parser is very popular and the dev/test dependency should not be a problem.

Copy link
Owner

Choose a reason for hiding this comment

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

MyST parser is very popular and the dev/test dependency should not be a problem.

Fair enough, this is the first time I've encountered it.


.. php:method:: returnGlobalConstant()

:returns: The value of a specific global constant.
:returns: The value of a specific global constant. # TODO link is not working without "\\"
Copy link
Owner

Choose a reason for hiding this comment

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

What's up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:returntype: SOME_CONSTANT return type reference with constant is unresolved (has no clickable link) - I need to find a solution first. If ok, let's merge this big PR and these small TODOs can be fixed later, probably together with #56.

@mvorisek
Copy link
Contributor Author

@markstory the changes are not smaller, but the most of the changed LoC are changes from generated expected html and changes due Black CS. If you want the the Black CS changes as separate PR, let me know, but as I see you merge PRs using merge, thus the individual commits will be preserved even if merged as one PR.

@markstory
Copy link
Owner

I had to turn off tests for python 3.9+ as they started failing. I've also turned off the cron schedule for CI as it has been failing for a few days, likely due to a change in sphinx.

@markstory markstory merged commit f785b8b into markstory:master Aug 18, 2023
2 checks passed
@mvorisek mvorisek deleted the fix_ns_resolving branch August 18, 2023 06:30
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.

Parse error must emit build warning
2 participants