develooper Front page | perl.perl5.porters | Postings from June 2013

Re: I made t/podcheck.t less sensitive and fixed various pod issues

Thread Previous | Thread Next
From:
Karl Williamson
Date:
June 7, 2013 20:42
Subject:
Re: I made t/podcheck.t less sensitive and fixed various pod issues
Message ID:
51B2458D.1040804@khwilliamson.com
On 05/23/2013 09:32 PM, David Golden wrote:
> On Thu, May 23, 2013 at 10:44 PM, Karl Williamson
> <public@khwilliamson.com> wrote:
>> I object to this being done without adequate discussion on the list.
>> It's not in keeping as to how this project has operated while I've been
>> involved, and is contrary to how I would want to have a project operate that
>> I contribute to.
>
> Has the code base been made worse?  No.  In fact I fixed a host of
> actual known pod issues that had been ignored that no one had bothered
> to fix.

The reason not to commit a likely contentious patch without prior 
vetting on p5p, is that it comes across as a power play designed to get 
one's way without regard to any merits.

Making the code base worse or not is irrelevant to my point.  Such 
mistakes can be fixed.  It is harder to fix the broken trust and broken 
relationships caused by power plays, which also drive away contributers 
who don't have the stomach for such things.  Scwhern's keynote at the 
Madison YAPC was about this sort of thing.

If a commit wasn't a power play, a good response, IMO, is "Oh.  I'm 
sorry.  I didn't realize this would be contentious.  Let's talk...."

So, let's talk about your email and the patches.

In fact, your patches do make the code base worse, for varied reasons. 
To summarize: they hinder ongoing efforts to fix some pod issues, 
including the complete removal of working, admittedly useful, tests from 
the core, for essentially aesthetic reasons.  Some of the pod fixes make 
the pods render worse, and some merely unflag things that are clearly 
wrong, leading to less probability that they will ever get fixed properly.

Further, podcheck.t was left it in an inconsistent state, with its own 
pod no longer matching what it currently does.  And there is this line 
now in it:

  my $MAX_LINE_LENGTH = 100;   # 79 columns

(The 100 used to be '79'.)  A future maintainer will be confused as to 
how 100 maps to 79, and potentially waste their time trying to figure it 
out.  This has the mark of a patch made in haste without really reading 
the code.  There was no inherent reason for haste in these patches.

>
> This is my beef with podcheck:  It's far to easy to let known issues
> grow and be ignored.

What you're complaining about is the pod equivalent of TODO tests.  TODO 
tests have long been recognized as a valid way to handle known issues. 
If this is your real beef, it would be inconsistent if you don't have 
the same beef with Test::More, t/test.pl and t/re/re_tests, because all 
allow fairly easy methods to mark tests as TODO, which grow and get ignored.

And, it appears to me that "far to easy to ignore" contradicts one of 
your stated motivations for these patches, that podcheck fails on 
problems too trivial to hold up development for.  If it's easy to 
ignore, then there shouldn't be an objectionable hold-up of development.

And it's simply not true that the number of known issues grows.  In fact 
they have drastically declined from release to release (the actual 
counts are below).
>
> If you don't like 100, go fix all the places that are over 100.

The only valid number is 79.  It is not 100 nor 90 nor 80.

(For those of you following at home, pagers that perldoc typically calls 
reserve a single column for their use, so that a pod that uses a maximum 
of 79 columns will fit without wrapping into a standard 80 column 
terminal window.  The pod formatters wrap non-verbatim lines properly, 
but they leave verbatim lines alone, hence podcheck looks for these, and 
only these (contrary to some lies that have been spread about it).  This 
issue would all or mostly go away if the pod language had the capability 
to make tables with rows and columns, and figured out how best to fit 
things into the space available.  But no one has yet to implement that.)

> Then *GET RID OFF* the known issues file.

This file cannot go completely away.  If we got rid of it, then these 
kinds of errors really would hold up development.  By having it, we 
allow people who don't have the tuits, or don't care about the issues 
shown, or just think that this kind of thing is beneath them, to go 
ahead and be that way, while others, such as myself. who don't think 
their task is complete until it is documented, and that documentation 
looks ok, can do our thing.  And people who want to come in and clean up 
things can easily find things to clean up.

And, there are cases where the 79 character limit is not feasibly met. 
Some will be cited below.

Without this file, it becomes tempting to take shortcuts just to get the 
test to pass, but leave the pods flawed, turning known, flagged problems 
into "unknown" problems.  I will argue below that is precisely what you 
succumbed to in some of your fixes in these commits.

This file functions as an automatically generated TODO (with some 
Won't-fixes scattered in).  If your argument were valid, it would mean 
that we should remove the capability of having TODO tests everywhere.

Indeed, I think it would be useful at times to have a greppable list of 
all the TODO tests in the core, something this file provides 
automatically for the pod ones.

>
> Then tighten the limit down to 90.  Then go fix all the places over
> 90.  Repeat until a point of diminishing returns.

Again, the only valid number is 79.  Anything else is useless.  It is 
not a matter of diminishing returns.  The only valid number is 79.

Suppose we take your suggestion.  And get everything to pass at 100. 
Then we tighten the limit to 90.  Some of the things that we just spent 
time on getting to <= 99 now fail, and we have to work on them again. 
It would have been far better to make those 79 in the first place.  But 
your patches make it harder to know if we did get to 79.
>
> If we *really* thought 80 characters was important, we'd *FIX* the
> problem, not use a test that is so easily bypassed.

But we are fixing them.  And again, it appears you are contradicting 
yourself.

>
> Karl -- you were responsible for most of the recent commits to
> pod/perlebcidic.pod.  Did you fix the many lines over 80?  No, you
> didn't.

This is more evidence of hastiness on your part.  If you had 
investigated before making this false claim, you would have found that 
more than 95% of the too-long verbatim lines in this pod have been 
fixed, all or nearly all by me, from 297 occurrences before I started, 
down to 13 in 5.18.0.  I didn't fix the remaining ones because at the 
time it seemed to me that wrapping was the least worst option.  All are 
regular expressions that won't fit in 79 columns even without indents, 
and most are unsplittable [bracketed character classes] (one has 96 
characters).  One could create variables for substrings of each class, 
and then write them like qr/[$a$b$c]/, but that would detract from their 
meanings, and is, IMO, a worse option than having the line wrap.  Things 
aren't perfect, and the known issues data base allows us to have 
imperfections without holding up development.
>
> I really respect your many contributions to the core, but please walk
> the walk before you talk the talk.

I believe I am not a hypocrite in this matter.  But I do think that your 
arguments are contradictory.
>
> Tests that flag things we don't fix aren't constructive.
>

But we are fixing them.  Not every one, and not immediately, but in the 
two years since podcheck has been more or less in its current form, the 
numbers have declined drastically.  Here they are:

5.14.0 (before the new podcheck):
# 1       =head2 without preceding higher level
# 7       =item type mismatch
# 35      ? Should you be using F<...> or maybe L<...> instead of
# 144     ? Should you be using L<...> instead of
# 286     Apparent broken link
# 44      Apparent internal link is missing its forward slash
# 4       No items in =over / =back list
# 2       Pod NAME already used
# 1       Should have =encoding statement because have non-ASCII
# 1       There is more than one target
# 4       There is no NAME
# 2076    Verbatim line length including indents exceeds 79 by
# 1       Verbatim paragraph in NAME section
# 10      empty section in previous paragraph
# 3       nested commands F<...F<...>...>
# 2       node contains non-escaped | or /
# 6       unresolved internal link
# -----
# 2627    known potential issues


5.16.0
# 1       =head2 without preceding higher level
# 7       =item type mismatch
# 12      ? Should you be using F<...> or maybe L<...> instead of
# 104     ? Should you be using L<...> instead of
# 20      Apparent broken link
# 17      Apparent internal link is missing its forward slash
# 4       No items in =over / =back list
# 2       Pod NAME already used
# 1       There is more than one target
# 4       There is no NAME
# 1551    Verbatim line length including indents exceeds 79 by
# 1       Verbatim paragraph in NAME section
# 7       empty section in previous paragraph
# 3       nested commands F<...F<...>...>
# 4       unresolved internal link
# -----
# 1738    known potential issues


5.18.0
# 7       =item type mismatch
# 12      ? Should you be using F<...> or maybe L<...> instead of
# 95      ? Should you be using L<...> instead of
# 3       No items in =over / =back list
# 2       Pod NAME already used
# 1       There is more than one target
# 2       There is no NAME
# 959     Verbatim line length including indents exceeds 79 by
# 1       Verbatim paragraph in NAME section
# 7       empty section in previous paragraph
# 3       nested commands F<...F<...>...>
# 4       unresolved internal link
# -----
# 1096    known potential issues


I don't see growing numbers in any of the categories.  I believe this 
conclusively demonstrates that you are mistaken in your belief that we 
are not fixing these.

I, and others, have been fixing things shown on this list, somewhat 
slowly, but surely.  These aren't severe enough to hold up development, 
but most of them should be fixed eventually.  But the number can never 
get to 0, and hence the data file will continue to be required.

Your patches to podcheck now make it harder to continue to whittle the 
remaining issues down.

Your patches completely remove the code that generates the lines:
  ? Should you be using F<...> or maybe L<...> instead of
  ? Should you be using L<...> instead of

I agree that the second one is lint-like and could be more appropriately 
done in some other Porting test.  I don't agree about the first one. The 
pod language says that file paths should be marked-up using F<>; it's an 
error not to do so,  I believe that this check does a good job of 
detecting this flaw.  It is a heuristic, so can have false positives and 
so needs the database to override that, but it has saved me a few times 
from committing pod changes with this error in them, and I have not seen 
or heard of it missing things that it should have picked up.

Once this check is there, the code to implement the other check falls 
into place easily.  I have examined the logs back to when the new 
podcheck stabilized, about two years ago.  In that time, there was only 
one commit that needed to happen because of this check, 
4c347e8605390be7efd8c5c530639eeaa097864e.  That commit did not result in 
Jenkins complaining, because the committer properly fixed things before 
pushing.  That tells me that this test, while perhaps more appropriate 
elsewhere, is essentially harmless here, and leaving it here allows 
easier maintenance of it.  You said its advice was ultimately useful, 
and could still be done periodically by those so inclined, but you made 
that impossible by completely removing it, providing no recourse.   I do 
very strongly believe that p5p should be queried before removing working 
functionality from the core.  That is common sense and common courtesy 
to your fellow porters.

Now let's consider the changes you made to the pods to get things to 
pass.  I have looked at most of those failures in the past, and for 
various reasons decide to leave them as TODOs for the time being.

Part of your patch removes the Z<> components which were used to make an 
=item in a definition list not appear to be a bullet or numbered list 
when it otherwise would be.  The Z<>s were in fact correct, and using 
them is the recommended method to accomplish this.  The pod formatting 
code correctly handles them.  That they showed up as errors is due to a 
bug in the old Pod::Checker, which was supposed to be replaced in 5.17, 
and now 5.19.  I'm told the new version handles these correctly.  Your 
patch converts the ones in perluniprops to a bullet list.  The user 
experience reading that is slightly worse with your changes than 
without.  That is why I didn't change them myself.  This is another 
reason we need the known issues file, to compensate for bugs in the 
checking software.

Your patch to configpm, which is used to generate lib/Config.pod causes 
the single instance of the message
	nested commands F<...F<...>...>
to go away.  It changes the generated pod to look like:
	F<$incpath/usr/include>
This is probably ok, but it is not the ideal solution.  '$incpath' is 
not a file, but a variable and ideally should be rendered differently 
than the literal text, perhaps like
	 F<I<$incpath>/usr/include>
Fixing this required more tuits than I was willing to expend when I 
looked at this issue, so I left it as a TODO, and I still believe it's 
better to leave it as a TODO; but I don't have strong feelings on the issue.

You also patched dist/Math-BigRat/lib/Math/BigRat.pm.  It had a line in it:
	See also L</blog()>.
but there is no link target "blog()".  You changed it to:
	See also C<blog()>.
This just sweeps the problem under the carpet.  I earlier went through 
this file and fixed the other broken links, spending some time figuring 
out what was meant for each. But I did not know what to do about this 
one, even after some investigation, so I left it flagged as a TODO.  Try 
to put yourself in the shoes of a customer reading this pod and coming 
upon this line. Imagine their frustration at trying to figure out what 
this means.  If you search the pod for "blog", the only other occurrence 
is in a list of unimplemented or not fully implemented functions.  (The 
text is unclear to me which as to which category this one falls into.) 
During the lifetime of this pod, this line will be read potentially many 
many times, causing frustration many many times.  It would be best to 
fix this.  I didn't have the tuits to spend on really getting to the 
bottom of this bug, and I wouldn't expect you to either.  But it would 
be better to leave it flagged as a TODO, which improves its chances of 
being noticed and fixed by someone who knows what they are doing.  But 
just changing it to something wrong just to not have a TODO test lowers 
its chances of getting fixed right, and does not have our customers' 
interests in mind.

I'll not comment on the other pod changes you made.  I haven't looked at 
them in detail; the ones above stood out to me.  I will say that I would 
have made the same changes you did to some of these others if I had felt 
confident that I knew how they were used in the development process 
(being internal pods, I believe, and not exposed outside).

So yes, portions of your patches left the code state worse than before, 
and those portions should be reverted.

But to return to my main point, making commits of likely contentious 
patches without prior public p5p discussion is almost guaranteed to hurt 
the project overall.  Collaborative environments are more productive, 
bikeshedding notwithstanding.


Thread Previous | Thread Next


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About