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

Bootstrap ext/random docs with docgen #1916

Merged
merged 17 commits into from
Oct 28, 2022
Merged

Bootstrap ext/random docs with docgen #1916

merged 17 commits into from
Oct 28, 2022

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Oct 25, 2022

  • The docs for the classes are not built yet, not sure where they need to be referenced to be included.

@TimWolla
Copy link
Member Author

TimWolla commented Oct 25, 2022

Okay, it appears that the documentation is completely unprepared to deal with a namespaced extension. When I reference &reference.random.random.engine; in book.xml, I need to fix the generated reference/random/random.engine.xml to reference &reference.random.random.entities.engine; instead of &reference.random.entities.random-engine;, but then all the methods of the actual engines are listed in the manual page for the Random\Engine interface:

image

@cmb69
Copy link
Contributor

cmb69 commented Oct 25, 2022

Thank you for the PR!

Minor issue: there should be an accompanying PR for doc-base which add the random book to manual.xml.inc. In the Gist you create a new <set>, which might be overkill. And maybe add it to the math extensions instead of the basic extensions.

Okay, it appears that the documentation is completely unprepared to deal with a namespaced extension.

Well, that is definitely tricky, but may work; see e.g. https://www.php.net/parallel and https://www.php.net/manual/en/book.ui.php.

Anyhow, I'll have a closer look tomorrow.

@TimWolla
Copy link
Member Author

And maybe add it to the math extensions instead of the basic extensions.

Ah, missed the "Other Basic Extensions" catch-all category. I believe that one is more appropriate than math, because it's not so much random number generation, but rather randomness generation that is not necessarily related to anything math.

Anyhow, I'll have a closer look tomorrow.

Thanks! Feel free to push necessary fixes directly into the TimWolla:ext-random branch. I've intentionally left the checkbox to allow maintainer updates checked.

Copy link
Contributor

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

I've hooked up the class and interface pages. (feel free to re-order). Now the actual work can happen. :)

reference/random/book.developer.xml Outdated Show resolved Hide resolved
reference/random/book.xml Show resolved Hide resolved
@cmb69
Copy link
Contributor

cmb69 commented Oct 26, 2022

Okay, it appears that the documentation is completely unprepared to deal with a namespaced extension.

There is a problem if there is a class and a namespace with the same name. In that case the class must not use the auto-generated entity, but rather has to use the individual entities of its methods. This should now be resolved (see random.engine.xml near the end of the file).

@TimWolla
Copy link
Member Author

There is a problem if there is a class and a namespace with the same name. In that case the class must not use the auto-generated entity, but rather has to use the individual entities of its methods. This should now be resolved (see random.engine.xml near the end of the file).

Not great, but acceptable as the interface is no likely to change in the future.

I've hooked up the class and interface pages. (feel free to re-order). Now the actual work can happen. :)

Should we already merge + deploy once your URL question is resolved? Having at least the stubs is already of some value and with the bootstrapping done it is easier to work on separate pages independently in different PRs.

@cmb69
Copy link
Contributor

cmb69 commented Oct 26, 2022

Should we already merge + deploy once your URL question is resolved?

I think we should have at least some real documentation. It doesn't have to be complete, but if we merge this, we easily end up in the same boot as with ext/intl, for instance, we're a lot of methods are still not documented after ~10years.

It may make sense to spread the word about ext/random needing documentation. Others could drop in patches (as comments), or push directly to the base branch.

@TimWolla
Copy link
Member Author

I think we should have at least some real documentation.

Okay, for a start I've added short descriptions to all methods and descriptions to all important classes.

It doesn't have to be complete, but if we merge this, we easily end up in the same boot as with ext/intl, for instance, we're a lot of methods are still not documented after ~10years.

And if we do not merge this, we don't have any documentation at all. Not sure if this is better 😃 In any case, being listed as a maintainer in EXTENSIONS, I plan to continue working on the ext/random documentation as time permits.

It may make sense to spread the word about ext/random needing documentation. Others could drop in patches (as comments), or push directly to the base branch.

This would definitely make sense. However I believe this is going to be much easier if users can see the rendered result, because pain points and issues are immediately visible there, whereas it's non-obvious with the XML format. It would also allow users to send simple PRs for single methods that can easily be reviewed and quickly merged, without having a 60-file monstrosity.

@cmb69
Copy link
Contributor

cmb69 commented Oct 27, 2022

Thank you! That looks already much better now. I'm willing to give this experiment (commit just skeletons in the hope the community fleshes these out in a timely manner) a try, but like to get feedback from translators whether this might be considerably more work for them. So @mumumu, @saundefined, @Girgias, what do you think about this?

@TimWolla TimWolla marked this pull request as ready for review October 27, 2022 16:28
@saundefined
Copy link
Member

Looks good, thank you!

whether this might be considerably more work for them

I don't think that's a problem

As a last resort, we can don't need to translate the extension until it is documented, then the English version is taken.
IMO it's better, than we'll not publish documentation for extension at all, and waiting for full description.

--

Aside:

It doesn't have to be complete, but if we merge this, we easily end up in the same boot as with ext/intl, for instance, we're a lot of methods are still not documented after ~10years.

It may make sense to spread the word about ext/random needing documentation. Others could drop in patches (as comments), or push directly to the base branch.

Maybe we'll create an issue(s) for undocumented extensions?

I believe some people want to help, but maybe it's not clear how to start and how they can help 🤔

What do you think?

@mumumu
Copy link
Member

mumumu commented Oct 28, 2022

but like to get feedback from translators whether this might be considerably more work for them

This is not a problem for me, too.
For active translators, it is not a major problem, maybe.

My concern is that ext/random author is already working on its documentation.

Can we merge this PR? @zeriyoshi.

@zeriyoshi
Copy link
Contributor

@mumumu @TimWolla
Thank you very much. Obviously my work was delayed due to my limited understanding of the PHP documentation system.
I would like to move forward with the documentation, but I am not confident in my ability to properly translate it into English. It will still take a bit of time, but I am willing to work on it.

Sorry about my lack of response for a while on many fronts. I have been away from the work too much and have missed the timing to return. I'm over a major climax in both my personal and professional life, so I'm good to go.

@zeriyoshi
Copy link
Contributor

Ok, I would totally like to use this bootstrap.
Please merge it!

@cmb69 cmb69 merged commit 2166824 into php:master Oct 28, 2022
@TimWolla TimWolla deleted the ext-random branch October 28, 2022 13:23
@Girgias Girgias added this to the PHP 8.2 milestone Nov 12, 2022
tiffany-taylor pushed a commit to tiffany-taylor/doc-en that referenced this pull request Jan 16, 2023
claudepache pushed a commit to claudepache/php-doc-en that referenced this pull request Jun 1, 2023
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

6 participants