[Pki-devel] [PATCH] 71-3 Patch for ticket 316- Adding pylint to the build process - Revised version of 71-2

1. There is a super long line in the compose script. Please break that into multiple lines. + echo -e "If the reported issues are false positives, please mark them ignored \nin the configuration file '$PKI_PWD/$PKI_DIR/dogtag.pylintrc' \nand build again." 2. The logic for determining whether or not to stop the script based on the return value is incorrect. +#OUTPUT STATUS CODE +# Pylint should leave with following status code: +# * 0 if everything went fine +# * 1 if a fatal message was issued +# * 2 if an error message was issued +# * 4 if a warning message was issued +# * 8 if a refactor message was issued +# * 16 if a convention message was issued +# * 32 on usage error +# +# status 1 to 16 will be bit-ORed so you can know which different categories has been issued by analysing pylint output status code + +result=0 +if [ $(($status&1)) -eq 1 ] || [ $(($status&2)) -eq 2 ] || [ $(($status&4)) -eq 4 ] +then + result=1 +fi what happens if there is an error and a warning? Then the return value will be 6. You need to decompose the error status and determine which bits were set. Ade On Mon, 2013-07-29 at 11:56 -0400, Abhishek Koneru wrote: > PFA the patch which addressed all comments mentioned below. > Raised a ticket for question number 8. > > --Abhishek > > On Fri, 2013-07-26 at 16:13 -0400, Abhishek Koneru wrote: > > A few questions/ comments: > > > > 1. In your build, you create a pylintscan directory within the source > > tree under pki. I would prefer this to take place in the build area so > > that it does not clutter up your source tree. The pylintscan is a build > > artifact. > > > > -- Sure can be moved. I can delete the folder once the scan is done. since the scan actually takes place before > > the build. (As per answer 2 - can i still keep pylintscan in pki?) > > > > 2. Do you need to remove the directory prior to each run? > > > > -- Not required. I will remove it in the script. Will just leave the pylint-report. > > Is it ok to put the report in the root directory of the code tree - pki? > > > > 3. What parts of the pylintrc are not default? ie. which parts did you > > change? > > > > -- The disable-ids property. (Will not disable conventions [C] and refactor[R].) > > -- Max lines - from 80 to 100 > > -- Removed 'string' package from deprecated modules. > > -- Allowing variables to be named 'rv'. (property - good-names) > > -- Set property - include ids = yes. > > > > 4. I noticed that you added JSONEncoder to the pylintrc as a class for > > which private members should not be checked. Does that allow you to > > eliminate the comment to # pylint: disable-msg=E0202 in encoder.py? > > > > -- Forgot to squash a few final changes made to pylintrc. it was actually not included in pylintrc. > > > > 5. Can you explain the rationale for ignoring W0511 and W0105? > > W0105 (pointless-string-statement): > > String statement has no effect Used when a string is used as a statement (which of course has no effect). This is a particular case of W0104 with its own message so you can easily disable it if you’re # using those strings as documentation, instead of comments. > > W0511 (fixme): Used when a warning note as FIXME or XXX is detected. > > > > -- Will add the same to the comments for disabled ids. > > > > 6. I would prefer to not ignore Convention, refactor etc. and to have > > these show up in the pylint reports. Based on the return code for > > pylint, you can have the build fail only if the bits for errors and > > warnings are set. > > > > -- Can be done. will make the change. > > > > 7. The pylint command line seems to have all the python executables > > listed -- is this required? It means having to update this list every > > time an executable is added, which is a step that can easily be missed. > > Can you just point the scan to the top level directory? > > > > -- Yes. Did not notice that Endi added __init__.py in all directories. Will change that. > > > > 8. Similarly to the point above, is there a way to scan the source tree > > to find all the python code? > > > > -- Yes it can be done. But since the number of source files is very small, i used this approach of copying > > the directories directly instead of scanning all the code. > > > > On Mon, 2013-07-22 at 21:32 -0400, Abhishek Koneru wrote: > > > Please review the patch which adds a script and also the pylint > > > configuration file to the code tree. The script is called in the compose > > > script for core packages before the actual packaging is done. If any > > > errors or warnings are reported by pylint, the build fails. > > > > > > I did not add pylint as part of build-requires in the spec file for > > > pki-core, but have put a check in the script to bypass trying to scan if > > > pylint is not installed but with a comment stating the same in the log. > > > > > > --Abhishek > > > _______________________________________________ > > > Pki-devel mailing list > > > Pki-devel(a)redhat.com > > > https://www.redhat.com/mailman/listinfo/pki-devel > > > > > > > > _______________________________________________ > > Pki-devel mailing list > > Pki-devel(a)redhat.com > > https://www.redhat.com/mailman/listinfo/pki-devel >