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.

Sign up for GitHub

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

Jump to bottom

autodoc: Handle apidoc continuation lines #22373

Closed
wants to merge 1 commit into from
Closed

autodoc: Handle apidoc continuation lines #22373

wants to merge 1 commit into from

Conversation

khwilliamson
Copy link
Contributor

@khwilliamson khwilliamson commented Jul 3, 2024 •
edited
Loading

With this commit, it is now possible to have continuation lines on apidoc lines in the source

@jkeenan
Copy link
Contributor

jkeenan commented Jul 3, 2024

For consistency with all your other "Combine API entries" pull requests, I believe that the changes to autodoc.pl itself (de6c30e) should go into a separate pull request.

@jkeenan
Copy link
Contributor

jkeenan commented Jul 3, 2024

The Subject line for this p.r. is slightly misleading. Last month, in this commit:

commit ee4e4f68dd9b1c3924f0750b92245e6855f12778
Author:     Karl Williamson <[email protected]>
AuthorDate: Mon Jun 10 10:37:02 2024 -0600
Commit:     Karl Williamson <[email protected]>
CommitDate: Wed Jun 12 14:45:11 2024 -0600

    perlapi: Combine sv_catpvf-type functions into one group

... you grouped these API functions together:

    "sv_catpvf"
    "sv_catpvf_nocontext"
    "sv_catpvf_mg"
    "sv_catpvf_mg_nocontext"
    "sv_vcatpvf"
    "sv_vcatpvf_mg"
    "sv_vcatpvfn"
    "sv_vcatpvfn_flags"

Can you briefly note why they should not go into the section discussed in this pull request? (I concede that that would make this section very large.)

ilmari
ilmari reviewed Jul 3, 2024
View reviewed changes
autodoc.pl Outdated
# Count lines easier
my $get_next_line = sub { $line_num++; return <$fh> };
# Count lines easier and handle apidoc continuation lines
my $get_next_line = sub { my $contents = <$fh>;
Copy link
Member

Choose a reason for hiding this comment

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

Can we wrap this after the { and indent the sub body the normal amount instead of half a screen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to preserve pre-existing style, but sure. Now done

ilmari
ilmari reviewed Jul 3, 2024
View reviewed changes
autodoc.pl Outdated
/$1/x)
{
my $next = <$fh>;
last unless defined $next;
Copy link
Member

Choose a reason for hiding this comment

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

This needs a no warnings 'exiting';

Copy link
Contributor Author

@khwilliamson khwilliamson Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Your comment doesn't make sense to me. The last is exiting a loop. The code works as-is without warnings being generated

@tonycoz
Copy link
Contributor

tonycoz commented Jul 4, 2024

Can you briefly note why they should not go into the section discussed in this pull request? (I concede that that would make this section very large.)

They do different things.

The functions described here are like $x .= "somestring"; or $x .= $y;

The pvf functions concatenate a formatted string, so like $x .= sprintf($format, $arg, ...); with all the complexity of the format string.

@khwilliamson
autodoc: Handle apidoc continuation lines
73dd2c5 
With this commit, it is now possible to have continuation lines on
apidoc lines in the source
@khwilliamson khwilliamson changed the title perlapi: Combine all sv_catpv() forms into one group autodoc: Handle apidoc continuation lines Jul 17, 2024
@khwilliamson
Copy link
Contributor Author

For consistency with all your other "Combine API entries" pull requests, I believe that the changes to autodoc.pl itself should go into a separate pull request.

I included both so as to prove that the change worked, as it was never exercised before. But I'm changing this PR to be the prerequisite for thesv_catpv() one, which I am deferring for now.

@khwilliamson khwilliamson mentioned this pull request Jul 17, 2024
Merged
tonycoz
tonycoz reviewed Jul 18, 2024
View reviewed changes
autodoc.pl
return unless defined $contents;

$line_num++;
while ($contents =~ s/ ^ ( =for \ apidoc .*) \s* \\ \n /$1/x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This allows only a single space between =for and apidoc, but the match in the loop below allows 1 or multiple.

This line should remove the \n but you also chomp below.

It might be good to allow extra whitespace between the \\ and the newline to avoid trailing whitespace looking like it should continue to a person reading the code, while autodoc.pl doesn't treat it as one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These points have made me close this request, and integrate the relevant portions into some commits in #22377

@khwilliamson khwilliamson deleted the catpv_api branch July 19, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonycoz tonycoz tonycoz left review comments

@ilmari ilmari ilmari left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khwilliamson @jkeenan @tonycoz @ilmari