8:32 p.m.
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
12:26 p.m.
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.
2. Do you need to remove the directory prior to each run?
3. What parts of the pylintrc are not default? ie. which parts did you
change?
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?
5. Can you explain the rationale for ignoring W0511 and W0105?
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.
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?
8. Similarly to the point above, is there a way to scan the source tree
to find all the python 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
3:13 p.m.
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
10:56 a.m.
New subject: [PATCH] 71-2 Patch for ticket 316- Adding pylint to the build process
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
3:43 p.m.
New subject: [PATCH] 71-2 Patch for ticket 316- Adding pylint to the build process
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
11:06 p.m.
New subject: [PATCH] 71-2 Patch for ticket 316- Adding pylint to the build process
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.
This might help:
How to check if the i-th bit is set in an integer
To check if a the i-th bit of a number n is set, we can see if ( n & 1
<< i ) is non-zero. To be precise if the bit is set, then the result
would be 2i. This is because in the number 2i there is only 1-bit and
that is the i-th bit, if it's ANDing with n results in a non-zero number
then the i-th bit is set in n.
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
>
_______________________________________________
Pki-devel mailing list
Pki-devel(a)redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
12:24 a.m.
New subject: [PATCH] 71-3 Patch for ticket 316- Adding pylint to the build process - Revised version of 71-2
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(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
>
4473
days inactive
4487
days old
6 comments
2 participants
participants (2)
-
Abhishek Koneru -
Ade Lee