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

Describe WeakMaps. #479

Merged
merged 7 commits into from
Apr 13, 2021
Merged

Describe WeakMaps. #479

merged 7 commits into from
Apr 13, 2021

Conversation

Crell
Copy link
Contributor

@Crell Crell commented Mar 4, 2021

The page was already there, but just auto-generated reference stuff. Now it explains what it actually does.

@cmb69
Copy link
Contributor

cmb69 commented Mar 5, 2021

I assume you want to document the new WeakMap class of the core, but this page is about PECL/weakref. I have no idea how to resolve that (search etc.)

@Crell
Copy link
Contributor Author

Crell commented Mar 5, 2021

Well carp. Yeah, I did. I figured this page was just auto-generated at some point and ran with it. How do we add a native version but avoid namespace collisions on IDs?

@cmb69
Copy link
Contributor

cmb69 commented Mar 5, 2021

Frankly, I have no idea. That was one of the reasons why I suggested to rename WeakRef to WeakReference, but I couldn't come up with another name for WeakMap. :(

@Crell
Copy link
Contributor Author

Crell commented Mar 7, 2021

Proposal: Rename class.weakref to class.ext-weakref, then create a new class.weakref page back in the "built-in" section that documents the one in core. The extension is unlikely to get much if any use in the future now that the functionality is built in.

Thoughts? Yay/nay?

@cmb69
Copy link
Contributor

cmb69 commented Mar 7, 2021

Not weakref but weakmap, though. :)

@Crell
Copy link
Contributor Author

Crell commented Mar 8, 2021

Right, that thing. 😄

Let's see if the bot likes this version.

(All of the new pages are copy-pasta from the extension, with the new description text. It looked like that was all accurate to me so I didn't bother changing anything.)

@Crell
Copy link
Contributor Author

Crell commented Apr 12, 2021

Rebased now that the old WeakMap class from the extension is gone. Bot appears happy.

@cmb69
Copy link
Contributor

cmb69 commented Apr 12, 2021

The pages are not used yet; you need to add &language.predefined.weakmap; to language/predefined/interfaces.xml. Then CI will likely fail. :)

Furthermore, there should be respective entries for the class and its methods in language/predefined/versions.xml.

@cmb69
Copy link
Contributor

cmb69 commented Apr 13, 2021

You need to list the entity references of the methods of Weakmap on its page, not the entity reference of the class itself:

 language/predefined/weakmap.xml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/language/predefined/weakmap.xml b/language/predefined/weakmap.xml
index 8619bdb878..69e3950da7 100644
--- a/language/predefined/weakmap.xml
+++ b/language/predefined/weakmap.xml
@@ -107,7 +107,13 @@ int(0)
 
  </partintro>
 
- &language.predefined.weakmap;
+ &language.predefined.weakmap.construct;
+ &language.predefined.weakmap.count;
+ &language.predefined.weakmap.getiterator;
+ &language.predefined.weakmap.offsetexists;
+ &language.predefined.weakmap.offsetget;
+ &language.predefined.weakmap.offsetset;
+ &language.predefined.weakmap.offsetunset;
 
 </phpdoc:classref>

Crell and others added 2 commits April 13, 2021 12:19
Co-authored-by: Christoph M. Becker <cmbecker69@gmx.de>
@cmb69
Copy link
Contributor

cmb69 commented Apr 13, 2021

So we're good to go. Thank you!

@cmb69 cmb69 merged commit bf28a4c into php:master Apr 13, 2021
@Crell Crell deleted the weakmap branch April 30, 2021 17:40
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

2 participants