Control: tags -1 pending Max-Julian Pogner: > tags +patch > > thanks > > (can i also send control commands here, or only to > cont...@bugs.debian.org? i will know after sending this email) > > > Hi Niels, >
Hi, Thanks for the follow up. :) You can do control messages when following up on the bug, but you have to prefix them with "Control: " (in addition to them being in the top). The bug number "-1" is in this case pre-sent to the bug you sent to. I have included an example above, which also serves to mark it as pending because I have merged your patch. Thanks for your contribution! :) >> I am inclined to go with option C by having `_strip_spaces` cope with >> its input being undefined and just immediately returning (or skipping >> the stripping part). >> > > I have created a patch and attached it to this mail. > Thanks, as mentioned I have merged it already. For future reference, I do have one Perl nit that I do not know if you are aware of for the following line: > + return undef if not defined($v); Perl has this weirdness where `return` and `return undef` behaves differently when the sub is called in `list` context. In the concrete case, it does not matter (as the sub is always called in `scalar` context). My nit is that I prefer to use `return` (without `undef`) for consistency that I am aiming towards (the existing code is not following 100% either as I recall). As mentioned (for this case), it is just a minor style thing and I have not bothered with it. But in case you are doing more debhelper patches (and I would welcome it if you did!), I wanted you to be aware of it. > Using command `debbuild` the package builds and i installed the > resulting deb package and it seems to work; plus one of the many > messages says: > >> All tests successful. > > So maybe all is fine? However my perl foo is not good enough to know how > to test whether a perl warning is issued. > > [...] > > > cya, > > Max Code-wise, this strongly resembles the fix I would have done and if it removes the warning for the cause that triggered this bug report, I am inclined to say it works. In theory, we could also do a test for it under t/Dh_Lib. However, it would involve a lot of work plus (I think) the Test::Warnings as a new test dependency. To be honest, I am not sure it is worth the hassle. In particular because Build-Dependencies have to be kept to a minimum - which we can work around with a bit of more effort, but I still think it is a lot of effort for something that is unlikely to regress. On a final note - thanks for taking your time to report the bug and providing the patch, which is now merged. I really appreciated that. :) Thanks, ~Niels
