3:43 p.m.
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