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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ devscripts-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/devscripts-devel
