licensecheck: improvements

Thu Oct 18 22:59:10 UTC 2012

Am Mittwoch, den 10.10.2012, 13:49 +1100 schrieb Dmitry Smirnov:
> On Wed, 10 Oct 2012 05:28:29 Benjamin Drung wrote:
> > 
> > * 0001-licensecheck-help-docs-update.patch:
> > 
> > "B<licensecheck> [options] filename [filename ...]" is the short form of
> > "B<licensecheck> [B<--no-conf>] [B<--verbose>] [B<--copyright>] [B<-l>|
> > B<--lines=>I<N>] [B<-i>|B<--ignore=>I<regex>] [B<-c>|<--check=>I<regex>]
> > [B<-m>|B<--machine>] [B<-r>|B<--recursive>] I<list of files and
> > directories to check>". Having both makes no sense.
> 
> Yes, I'm with you this is work-in-progress please feel free to trim output but 
> I would expect you to apply this patch first.

My usual approach is the other way around: Improve the patches
iteratively until I am happy and then apply them.

> > The current output of --help looks better to me IMO. It's shorter and
> > more precise. I expect a long and verbose text in the man page instead.
> 
> This is subjective but we can always change the output later.
> The importance of this patch is that it introduces better interface to show 
> help produced from information embedded in POD rather than have duplication. 
> This is quite important for maintainability.

I agree that reduce duplication is a good idea.

> > Before touching the command line interface, more command line parameter
> > tests would be nice (see test/test_licensecheck).
> 
> Sorry, I would prefer to focus on important issues and polish the code rather 
> than test the obvious. I never had need to test Getopt::Long interface -- what 
> makes you think we should spend out time for it in licensecheck??

Having command line parameter tests was not mandatory for applying the
patch (that why I wrote "would be nice"). The tests should test that
licensecheck behave correctly if specific parameters are given. The
Getopt::Long interface is then tested indirectly.

> > * 0003-licensecheck-removing-license-statement-from-version.patch
> > 
> > Adding "(at your opinion)" is okay. Maybe the lines count could be
> > reduced by one.
> 
> OK but this statement is unnecessary and unusual. As patch description says 
> licensecheck have three (!) license statements: in header, in POD and in 
> --version.

Other software has as many license statements, too: in header, in man
page, in --version.

> This is despite /usr/share/doc/devscripts/copyright
> which would be forth if you count it.
>
> I think two is more than enough. Wouldn't you agree that expected output of 
> --version would be ... version (without license statement). 
> This is just common sense.

Can you prove that it is common sense that --version does not show a
license statement? I tested cp and ls and both state a license, too.

> > * 0004-licensecheck-move-comments-clean-up-code-to-function.patch
> > 
> > The clean_comments function does more than just cleaning the comments.
> > It removes tabs and newlines for instance. Therefore a better name for
> > the function should be used. The return statement is not correctly
> > indented.
> 
> I'm not convinced that a different name should be used. This can always be 
> changed later and it is not an important issue. Usually I prefer commenting 
> every single function in code and I didn't quite do it yet.
> We may have different preferences regarding indentation but that's not Python 
> and therefore do not affect logic. I can indent as you wish -- you could just 
> express this after applying the patch or correct it before applying.

Is not indenting the return statement really a coding style? It look
more like an indentation mistake to me.

The patch didn't apply. Otherwise I could have corrected the
indentation.

> > * 0005-licensecheck-removing-function-prototypes-perl-best-.patch
> > 
> > Looks good, but it does not apply out of order.
> > 
> 
> That's an evidence that perhaps you could be less conservative with reviewing 
> patches. Otherwise we will unnecessary waste too much time like this...
> 
> If I can work in dedicated branch perhaps you could check to see how it goes 
> and do not block the progress unnecessarily.
> 
> You see if I do all the work as I see it and present you the result couple of 
> weeks later you would probably dismiss it due to large number of changes that 
> we did not discuss.
> 
> Frankly working through your too strict filters feels too difficult especially 
> if you prefer perfection in every commit.

I am not the author of licensecheck and Perl is not my favorite
programming language. So I may be more conservative applying patches.
That's why I may let you iterate over a patch until I am confident in
applying it.

> I'm a Perl developer and if you noticed my commits are made in a way that they 
> do not break things.
> 
> Having said this I understand that you may disagree on how did I change 
> commands output etc.
> 
> 
> > * 0007-properly-report-empty-license.patch
> > 
> > I like to have a test case for this before applying it.
> 
> Please forgive me but this is one line patch -- how can it be more 
> straightforward?!? 
> This is to abort license scan if input is an empty string after trimming 
> comments.
> 
> Test case for this is in the following commit that you also dismissed. :(
> 
> 
> 
> > 
> > * 0008-introducing-embedded-tests.patch
> > 
> > I like to see these test cases as shunit2 test added to
> > test/test_licensecheck instead of embedding them. Using the shunit2
> > framework has two advantages: It does not make the script slower (as
> > Perl is a interpreted language) and the I/O part is tested as well.
>  
> I don't know shunit2 framework and it is neither standard nor common for Perl.
> I don't like current tests structure and the output is very limited.
> 
> Yes I'm making script a *little* slower but as I mentioned in patch header 
> comments, tests are loaded only if activated with --tests option.
> 
> Perl stops loading file when found __DATA__ and I put tests beyond this mark.
> 
> I did some benchmarks (beforehand) and found it working as expected.
> 
> I/O part is completely irrelevant for tests but obviously loading all tests 
> from the end of script is faster than use many files.
> 
> There is a big advantage of having tests embedded in script.
> This make script self-contained and well compartmentalised.
> 
> This makes hacking and testing little patches much easier than checking out 
> packaging, understanding uncommon tests framework figure out how to run tests 
> (they do not invoked automatically on package build) etc.

The tests are run on package build!

> I would like to separate licensecheck testing from packaging and frankly until 
> we will have more than few hundred tests (if ever) if is very convenient to 
> have everything in one small file and therefore benefit from quick access.

I prefer the current test suite over the test in the licensecheck file
itself.

> > PS: Sorry for being a perfectionist.
> 
> No excuse. We won't make any progress like this.
> This is not perfectionism but protectionism as you effectively (de-facto) 
> protect licensecheck from any changes.

My comments on your patches are not meant to block you. It's to
improve/change the patches or further discuss the change with the other
maintainer and then apply your patch or later revisions of your patches.

> I still have hopes to convince you that I will make more good than harm.

I appreciate that you work on licensecheck and I do not think that you
want to do harm.

-- 
Benjamin Drung
Debian & Ubuntu Developer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.alioth.debian.org/pipermail/devscripts-devel/attachments/20121019/fd9e48c9/attachment.pgp>