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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

Please find attached a revised version of the patch to add pylint to dogtag's build process. The previous patch was added as part of the compose scripts but the current one is added to the spec-file. With this patch, building dogtag requires pylint to be installed on the machine. If any errors/warnings are reported by pylint, the build fails. The report can be found along with the build log in the packages folder. --Abhishek On Wed, 2013-07-31 at 16:43 -0400, Ade Lee wrote:

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@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel