licensecheck: improvements

[Dmitry Smirnov]

Hi Benjamin,

Thanks for having a look at the patches.

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.


> 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.

As far as I can see in the past having two help sections wan not very helpful.
It is more difficult to maintain and description of options has significantly 
diverged. I have impression that documentation was not updated for a while and 
that descriptions of options given in doc and in --help were written by 
different people.

I believe it is beneficial to encourage up-to-date documentation.


> 
> 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??


> 
> * 0002-licensecheck-new-option-print-defaults.patch
> 
> Depends on the previous patch. I don't need --print-defaults, but other
> may.
> 

Again this is to have consistent interface to command line arguments.

Since defaults are dynamically calculated we can't hardcode them to embedded 
docs. IMHO defaults do not belong to --help output hence I moved them to 
standalone function. This is another step to clean-up code a little and to 
make it more consistent.



> * 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.

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.

> 
> I dunno if the license should be stated if --version is called. What do
> the other maintainers think? Printing the copyright without the license
> is strange. Either print both or none. ls and cp for instance print the
> license.

You're right we probably should remove copyright as well.


> 
> * 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.

I'm very disappointed that this harmless and innocent patch was not applied.
It is absolutely critical to make things right in regards to automated 
testing.

> 
> * 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'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.


> * 0006-licensecheck-perl-best-practice.patch
> 
> Applied in jessie branch.

Thanks.


> 
> * 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.

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.



> * 0009-removing-w-as-we-already-use-warnings.patch
> 
> Applied in jessie branch.

Thanks.

> 
> 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.

I'm a perfectionist myself -- otherwise why would I bother polishing code and 
doing the change properly after introducing extensive test suite which nobody 
did before me?
I could just hack regex, attach the patch to the ticket and forget about the 
issue.

I might be CPP and Python illiterate but I have a decent experience with 
development in Perl and I know what I'm doing.
I would appreciate a little more trust to the work in progress.

Please express your wishes and I will implement.
Please feel free to correct things like indentation etc.

Maybe we could arrange a dedicated branch for my work?
I still have hopes to convince you that I will make more good than harm.

-- 
Regards,
Dmitry.