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