Paul, Thanks for the review. I have addressed all of your comments and posted an updated webrev at http://cr.opensolaris.org/~elafever/pylint/
Thanks, -Erik Paul Cunningham wrote: > Erik, > > See my comments below ... > > paul > > Erik Lafever wrote: >> >> I am working on the port for pylint, python code static checker. >> Pylint requires logilab-common and logilab-astng >> which have just recently been code reviewed. If I could get a few >> people to review the webrev, I would appreciated it. > > 1. > Lines ... > 49 chmod 444 $(PROTOMAN)/pylint.1 > 51 chmod 444 $(PROTOMAN)/pyreverse.1 > do you need this, as the following 'protofix' should > also correct it I think ? I removed those lines and protofix set the correct perms. > > Line .. > 41 gzip -dc $(TARBALL) | tar xmpf - > change 'tar' to '$(TAR)'. You also might want to think about > using the built in uncompress of the tarball rather than doing > it explicitly (personally I prefer it done explicitly though). I changed it to $(TAR) as I am not familiar with the built in uncompress. > > 'clean:' rule, do you also need to remove the $(LOCALPROTO) > stuff? the $(LOCALPROTO) is a subdirectory of $(VER) so it will be removed too. > > 2. usr/src/cmd/pylint/pylint.1.sunman > & usr/src/cmd/pylint/pyreverse.1.sunman > Is the the sunman stability stuff added to this when > installed, if not it needs to be added directly here > or using a sunman-stability sed script. I added the sunman stability to these files and verified the man pages after installing the package. > > 3. usr/src/pkgdefs/SUNWpylint/pkginfo > Delete this file as its generated from SUNWpylint/pkginfo.tmpl removed > > 4. usr/src/pkgdefs/SUNWpylint/pkginfo.tmpl > Line ... > 45 DESC="Python source code analyzer" > add the pkg version at the end, eg ... > DESC="Python source code analyzer (0.18.0)" added > > 5. man pages > Shouldn't there be man pages for the other installed binaries > also, ie. epylint, symilar & pylint-gui ? yes, I have added man pages for these binaries > > 6. usr/src/pkgdefs/SUNWpylint/prototype_com > Do not install stuff into /usr with the write permission > bits set, ie. lines 113-117 fixed > > END
