From 76993f44177b6d607ea39ad1ffe987a3e61327a2 Mon Sep 17 00:00:00 2001 From: Steve Hancock Date: Tue, 29 Dec 2020 06:48:24 -0800 Subject: [PATCH] improve side comment locations --- lib/Perl/Tidy/VerticalAligner.pm | 50 +++++++++--- local-docs/BugLog.pod | 118 +++++++++++++++++++++++++-- t/snippets/expect/comments.comments2 | 2 +- t/snippets/expect/comments.comments3 | 2 +- t/snippets/expect/comments.comments4 | 2 +- t/snippets/expect/comments.def | 2 +- t/snippets/expect/mangle4.def | 2 +- t/snippets/packing_list.txt | 2 +- t/snippets17.t | 8 +- t/snippets22.t | 2 +- 10 files changed, 162 insertions(+), 28 deletions(-) diff --git a/lib/Perl/Tidy/VerticalAligner.pm b/lib/Perl/Tidy/VerticalAligner.pm index 6a87ddc4..32fb0ea9 100644 --- a/lib/Perl/Tidy/VerticalAligner.pm +++ b/lib/Perl/Tidy/VerticalAligner.pm @@ -677,7 +677,7 @@ EOM # if none exists. # -------------------------------------------------------------------- $self->make_side_comment( $rtokens, $rfields, $rpatterns, $rfield_lengths, - $level_end ); + $level_end, $level ); $jmax = @{$rfields} - 1; # -------------------------------------------------------------------- @@ -789,12 +789,17 @@ sub join_hanging_comment { return 1; } +# Switch for comparing side comment forgetting methods. This can eventually +# be removed. +use constant TEST_OLD_SIDE_COMMENT_METHOD => 0; + sub make_side_comment { # create an empty side comment if none exists - my ( $self, $rtokens, $rfields, $rpatterns, $rfield_lengths, $level_end ) = - @_; + my ( $self, $rtokens, $rfields, $rpatterns, $rfield_lengths, $level_end, + $level ) + = @_; my $jmax = @{$rfields} - 1; @@ -810,15 +815,19 @@ sub make_side_comment { # line has a side comment.. else { - # don't remember old side comment location for very long - my $line_number = $self->get_output_line_number(); - if ( - $line_number - $self->[_last_side_comment_line_number_] > 12 + # don't remember old side comment location for very long... + my $line_number = $self->get_output_line_number(); + my $last_sc_level = $self->[_last_side_comment_level_]; - # and don't remember comment location across block level changes - || ( $level_end < $self->[_last_side_comment_level_] - && $rfields->[0] =~ /^}/ ) - ) + # OLD METHOD: forget side comment location across block level changes. + # NEW METHOD: forget side comment location unless old level matches + # either leading or trailing level. + my $is_level_change = TEST_OLD_SIDE_COMMENT_METHOD + ? $level_end < $last_sc_level && $rfields->[0] =~ /^}/ + : $level_end != $last_sc_level && $level != $last_sc_level; + + if ( $line_number - $self->[_last_side_comment_line_number_] > 12 + || $is_level_change ) { $self->forget_side_comment(); } @@ -1169,6 +1178,7 @@ sub check_match { $GoToMsg = "Not all tokens match: $imax_align != $jlimit\n"; goto NO_MATCH; } + } # The tokens match, but the lines must have identical number of @@ -1178,6 +1188,24 @@ sub check_match { goto NO_MATCH; } + # FOR TESTING ONLY: if we do not mix lines with and without side comments + # in the top-down sweep, then lines without side comments cannot influence + # side comment locations. The results of this test are interesting but not + # always better. But it could be that some combination of the two + # possibilities can give better overall results, so this test patch is + # temporarily retained for further experimentation. + if (0) { #<<< + my $sc_len_base = $base_line->get_rfield_lengths()->[$maximum_field_index]; + my $sc_len = $new_line->get_rfield_lengths()->[$jmax]; + if ( $sc_len == 0 && $sc_len_base > 0 || $sc_len > 0 && $sc_len_base == 0 ) + { + EXPLAIN_CHECK_MATCH + && print + "match but sc mismatch, imax_align=$imax_align, jmax=$jmax\n"; + return ( 1, $jlimit ); + } + } + # The tokens match. Now See if there is space for this line in the # current group. if ( $self->check_fit( $new_line, $base_line ) && !TEST_SWEEP_ONLY ) { diff --git a/local-docs/BugLog.pod b/local-docs/BugLog.pod index 125ce4fc..98eb5a06 100644 --- a/local-docs/BugLog.pod +++ b/local-docs/BugLog.pod @@ -2,12 +2,114 @@ =over 4 +=item B + +The code which aligns side comments remembers the most recent side comment and +in some cases tries to start aligning at that column for later side comments. +Sometimes the old side comment column was being remembered too long, causing +occasional poor formatting and causing a noticable and unexpected drift of side +comment locations to the right. The rule for forgetting the previous side +comment column has been modified to reduce this problem. The new rule is +essentially to forget the previous side comment location at a new side comment +with different indentation level or significant number of lines without side +comments (about 12). The previous implementation forgetting changes in +indentation level across code blocks only. Below is an example where the old +method gets into trouble and the new method is ok: + + # OLD: + foreach my $r (@$array) { + $Dat{Data}{ uc $r->[0] } = join( ";", @$r ); # store all info + my $name = $Dat{GivenName}{ uc $r->[0] } || $r->[1]; + + # pass array as ad-hoc string, mark missing values + $Dat{Data}{ uc $r->[0] } = join( + ";", + ( + uc $r->[0], uc $name, # symbol, name + $r->[2], $r->[3], $r->[4], # price, date, time + $r->[5], $r->[6], # change, %change + $r->[7], "-", "-", "-", # vol, avg vol, bid,ask + $r->[8], $r->[9], # previous, open + "$r->[10] - $r->[11]", $r->[12], # day range,year range, + "-", "-", "-", "-", "-" + ) + ); # eps,p/e,div,yld,cap + } + +The second side comment is at a deeper indentation level but was not being forgotten, +causing line length limits to interfere with later alignment. The new rule gives +a better result: + + # NEW: + foreach my $r (@$array) { + $Dat{Data}{ uc $r->[0] } = join( ";", @$r ); # store all info + my $name = $Dat{GivenName}{ uc $r->[0] } || $r->[1]; + + # pass array as ad-hoc string, mark missing values + $Dat{Data}{ uc $r->[0] } = join( + ";", + ( + uc $r->[0], uc $name, # symbol, name + $r->[2], $r->[3], $r->[4], # price, date, time + $r->[5], $r->[6], # change, %change + $r->[7], "-", "-", "-", # vol, avg vol, bid,ask + $r->[8], $r->[9], # previous, open + "$r->[10] - $r->[11]", $r->[12], # day range,year range, + "-", "-", "-", "-", "-" + ) + ); # eps,p/e,div,yld,cap + } + +The following exampel shows an unexpected alignment in the cascade of trailing +comments which are aligned but slowly separating from their closing containers: + + # OLD: + { + $a = [ + Cascade => $menu_cb, + -menuitems => [ + [ Checkbutton => 'Oil checked', -variable => \$OIL ], + [ + Button => 'See current values', + -command => [ + \&see_vars, $TOP, + + ], # end see_vars + ], # end button + ], # end checkbutton menuitems + ]; # end checkbuttons cascade + } + +This was caused by forgetting side comments only across code block changes. The new +result is more reasonable: + + # NEW: + { + $a = [ + Cascade => $menu_cb, + -menuitems => [ + [ Checkbutton => 'Oil checked', -variable => \$OIL ], + [ + Button => 'See current values', + -command => [ + \&see_vars, $TOP, + + ], # end see_vars + ], # end button + ], # end checkbutton menuitems + ]; # end checkbuttons cascade + } + +This change will cause occasional differences in side comment locations from +previous versions but overall it gives fewer unexpected results so it is a worthwhile +change. 29-Dec-2020. + =item B In rare cases it is necessary to update the type of lists, and this influences vertical alignment. This update fixes a minor inconsistency in doing this. In some rare cases with complex list elements vertical -alignment can be improved. +alignment can be improved. 27 Dec, 2020, 751faec. # OLD return join( '', @@ -59,7 +161,7 @@ below: The rule in sub 'two_line_pad' was updated to allow alignment of any lists if the patterns match exactly (all numbers in this case). Updated -27-Dec-2020. +27-Dec-2020, 035d2b7. =item B @@ -90,6 +192,8 @@ quotes. This update avoids this problem by not formatting such lists with the @trig, ); +27-Dec-2020, 948c9bd. + =item B This update adds a sequence numbering system for multiline qw quotes. In the @@ -165,6 +269,8 @@ Here is another example Class::XYZ::Overload ); #<-- outdented same as the line with 'for qw(' +26 Dec 2020, cdbf0e4. + =item B In the process of making vertical alignments, lines which are simple lists of @@ -206,7 +312,7 @@ For example (note padding after first comma) my ( $ym, $al, $cov, $bet, $olda, $ochisq, $di, $pivt, $info ) = map { null } ( 0 .. 8 ); -This update was made 22 Dec 2020. +This update was made 22 Dec 2020, 36d4c35. =item B @@ -266,7 +372,7 @@ but (';' remains indented): @trig ); -This update was added 18 Dec 2020 and modified 24 Dec. +This update was added 18 Dec 2020 and modified 24 Dec 2020, 538688f. =item B @@ -277,7 +383,7 @@ perltidy does not change whitespace. This update was added 17 Dec 2020. Moved inner part of sub check_match into sub match_line_pair in order to make info available earlier. This gave some minor alignment improvements. -This was done 16 Dec 2020. +This was done 16 Dec 2020, 7ba4f3b. # OLD: @tests = ( @@ -333,7 +439,7 @@ achieved by the subsequent 'sweep' pass. elsif ( $cmd eq "remove" ) { $remove = 1; last; } elsif ( $cmd eq "help" ) { $help = 1; last; } -This update was made 14 Dec 2020. +This update was made 14 Dec 2020, 44e0afa. =item B diff --git a/t/snippets/expect/comments.comments2 b/t/snippets/expect/comments.comments2 index 713e9d33..5eb3693a 100644 --- a/t/snippets/expect/comments.comments2 +++ b/t/snippets/expect/comments.comments2 @@ -32,7 +32,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 diff --git a/t/snippets/expect/comments.comments3 b/t/snippets/expect/comments.comments3 index 077d0818..eecb8204 100644 --- a/t/snippets/expect/comments.comments3 +++ b/t/snippets/expect/comments.comments3 @@ -48,7 +48,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 diff --git a/t/snippets/expect/comments.comments4 b/t/snippets/expect/comments.comments4 index 58638d2b..dee97983 100644 --- a/t/snippets/expect/comments.comments4 +++ b/t/snippets/expect/comments.comments4 @@ -49,7 +49,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 diff --git a/t/snippets/expect/comments.def b/t/snippets/expect/comments.def index 58ffcdf7..431e492c 100644 --- a/t/snippets/expect/comments.def +++ b/t/snippets/expect/comments.def @@ -46,7 +46,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 diff --git a/t/snippets/expect/mangle4.def b/t/snippets/expect/mangle4.def index ed062760..84d1986c 100644 --- a/t/snippets/expect/mangle4.def +++ b/t/snippets/expect/mangle4.def @@ -13,5 +13,5 @@ sub t086 ( #foo))) 333 #foo))) , #foo))) , #foo))) - ) #foo))) + ) #foo))) { $a . $b } diff --git a/t/snippets/packing_list.txt b/t/snippets/packing_list.txt index 78559e5b..158c7fda 100644 --- a/t/snippets/packing_list.txt +++ b/t/snippets/packing_list.txt @@ -307,6 +307,7 @@ ../snippets23.t align34.def ../snippets23.t git47.def ../snippets23.t git47.git47 +../snippets23.t qw.def ../snippets3.t ce_wn1.ce_wn ../snippets3.t ce_wn1.def ../snippets3.t colin.colin @@ -447,4 +448,3 @@ ../snippets9.t rt98902.def ../snippets9.t rt98902.rt98902 ../snippets9.t rt99961.def -../snippets23.t qw.def diff --git a/t/snippets17.t b/t/snippets17.t index 9bfe6368..ab2d0ba1 100644 --- a/t/snippets17.t +++ b/t/snippets17.t @@ -464,7 +464,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 @@ -545,7 +545,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 @@ -639,7 +639,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 @@ -738,7 +738,7 @@ sub macro_get_names { # { { { ${msg} = "Hello World!"; print "My message: ${msg}\n"; } - } #end level 4 + } #end level 4 } # end level 3 } # end level 2 } # end level 1 diff --git a/t/snippets22.t b/t/snippets22.t index 92563c25..99301159 100644 --- a/t/snippets22.t +++ b/t/snippets22.t @@ -549,7 +549,7 @@ sub t086 ( #foo))) 333 #foo))) , #foo))) , #foo))) - ) #foo))) + ) #foo))) { $a . $b } #11........... }, -- 2.39.5