perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

Document various newSUB functions

Open khwilliamson opened this issue 3 years ago • 3 comments

khwilliamson avatar May 08 '22 03:05 khwilliamson

On 5/10/22 07:25, iabyn wrote:

@.**** commented on this pull request.

Some general comments:

By upgrading a bunch of functions from 'Ap' to 'Apd', we are effectively making fully public, functions that used to be 'subject to change'. Are we sure we want to do this?

I'd actually prefer to leave them undocumented and mark them as internal. If that's ok with you, I'll do that instead.

khwilliamson avatar May 10 '22 18:05 khwilliamson

I'd actually prefer to leave them undocumented and mark them as internal.

I'm not really sure. I've always been uncomfortable with the status of functions marked as API but with no documentation. I think it would be best to leave as-is for 5.36 then reconsider. I guess have a look at current usage (if any) on CPAN to see if people are using them to construct optrees etc, then decide whether to make them API or not.

iabyn avatar May 11 '22 07:05 iabyn

@iabyn, here's what I think would be best for now. Leave the few that have cpan uses as-is, and mark the other ones as experimental. That makes them even more use-at-your-own-risk.

khwilliamson avatar May 16 '22 03:05 khwilliamson

On 5/11/22 01:29, iabyn wrote:

I'd actually prefer to leave them undocumented and mark them as
internal.

I'm not really sure. I've always been uncomfortable with the status of functions marked as API but with no documentation. I think it would be best to leave as-is for 5.36 then reconsider. I guess have a look at current usage (if any) on CPAN to see if people are using them to construct optrees etc, then decide whether to make them API or not.

I'm uncomfortable with the status quo as well, so am trying to cut the number of undocumenteds down. These stood out as fairly easy pickings.

There is a bit of usage on CPAN, CV and SV only. 10 distros, but only 8 results showed; I don't know if that is a bug with metacpan grep or what.

https://grep.metacpan.org/search?q=new%5BACGHS%5DVREF%5Cs*%5C%28&qd=&qft=*.xs

We could also mark them as public but experimental, subject to change. But it seems to me that the ship has sailed, and at least those two need to be marked public, and I would lean towards the others too.

khwilliamson avatar Oct 11 '22 07:10 khwilliamson

@iabyn do you still have concerns about this PR? @khwilliamson anything further needed here?

demerphq avatar Feb 08 '23 03:02 demerphq

Resolved conflicts in embed.fnc

demerphq avatar Feb 20 '23 15:02 demerphq

@iabyn do you still have concerns about this PR? @khwilliamson anything further needed here?

@iabyn, @khwilliamson, can you respond to @demerphq's questions ^^? Can this be merged?

jkeenan avatar Aug 01 '23 02:08 jkeenan

@iabyn do you still have concerns about this PR? @khwilliamson anything further needed here?

@iabyn, @khwilliamson, can you respond to @demerphq's questions ^^? Can this be merged?

Again: @iabyn, @khwilliamson, can you respond to @demerphq's questions ^^? Can this be merged?

jkeenan avatar Oct 17 '23 17:10 jkeenan

I'm not wild about it, but I guess its ok to merge.

iabyn avatar Oct 18 '23 10:10 iabyn