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

[Ade Lee]

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 at redhat.com
> > > https://www.redhat.com/mailman/listinfo/pki-devel
> > 
> > 
> > 
> > _______________________________________________
> > Pki-devel mailing list
> > Pki-devel at redhat.com
> > https://www.redhat.com/mailman/listinfo/pki-devel
>