Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-07 Thread Andrew Lazarev
 I can't say I'm too deeply versed in the code,  but it's enough to make
me wonder if we want to go that direction and avoid the issues altogether?

It's the nature of python that methods and modules can be added in runtime
and pylint can't do full analysis. That's why the best use of it - limited
list of checks applied to the last commit only. This is a way how nova and
sahara have it implemented. Non-voting gate job helps to find silly
mistakes that could barely be found by other ways.

Thanks,
Andrew.

On Fri, Oct 3, 2014 at 10:09 AM, Neal, Phil phil.n...@hp.com wrote:

  From: Dina Belova [mailto:dbel...@mirantis.com]
  On Friday, October 03, 2014 2:53 AM
 
  Igor,
 
  Personally this idea looks really nice to me, as this will help to avoid
  strange code being merged and not found via reviewing process.
 
  Cheers,
  Dina
 
  On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov
  idegtia...@mirantis.com wrote:
  Hi folks!
 
  I try too guess do we need in ceilometer checking new patches for
  critical errors with pylint?
 
  As far as I know Nova and Sahara and others have such check. Actually
  it is not checking of all project but comparing of the number of
  errors without new patch and with it, and if diff is more then 0 then
  patch are not taken.

 Looking a bit deeper it seems that Nova struggled with false positives and
 resorted to https://review.openstack.org/#/c/28754/ , which layers some
 historical checking of git on top of pylint's tendency to check only the
 latest commit. I can't say I'm too deeply versed in the code,  but it's
 enough to make me wonder if we want to go that direction and avoid the
 issues altogether?

 
  I have taken as pattern Sahara's solution and proposed a patch for
  ceilometer:
  https://review.openstack.org/#/c/125906/
 
  Cheers,
  Igor Degtiarov
 
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 
 
  --
  Best regards,
  Dina Belova
  Software Engineer
  Mirantis Inc.
 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-07 Thread Andrew Lazarev
At first step we won't implement pylint as gate job, but will add it at
master to have a possibility to check  code with pylint locally, if it is
needed.
No much sense in running it locally. It has too many false positives. The
best use - see new critical errors as a non-voting job.

Andrew.

On Mon, Oct 6, 2014 at 2:38 AM, Igor Degtiarov idegtia...@mirantis.com
wrote:

 My points are next:

 1. Pylint check will be very useful for project, and will help to
 avoid critical errors or mistakes in code.

 2. At first step we won't implement pylint as gate job, but will add
 it at master to have a possibility to check  code with pylint locally,
 if it is needed.

 3. In future it could be added as a non-voting job.
 -- Igor


 On Sat, Oct 4, 2014 at 1:56 AM, Angus Lees g...@inodes.org wrote:
  You can turn off lots of the refactor recommendation checks.  I've been
  running pylint across neutron and it's uncovered half a dozen legitimate
  bugs so far - and that's with many tests still disabled.
 
  I agree that the defaults are too noisy, but its about the only tool that
  does linting across files - pep8 for example only looks at the current
 file
  (and not even the parse tree).
 
  On 4 Oct 2014 03:22, Doug Hellmann d...@doughellmann.com wrote:
 
 
  On Oct 3, 2014, at 1:09 PM, Neal, Phil phil.n...@hp.com wrote:
 
   From: Dina Belova [mailto:dbel...@mirantis.com]
   On Friday, October 03, 2014 2:53 AM
  
   Igor,
  
   Personally this idea looks really nice to me, as this will help to
   avoid
   strange code being merged and not found via reviewing process.
  
   Cheers,
   Dina
  
   On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov
   idegtia...@mirantis.com wrote:
   Hi folks!
  
   I try too guess do we need in ceilometer checking new patches for
   critical errors with pylint?
  
   As far as I know Nova and Sahara and others have such check. Actually
   it is not checking of all project but comparing of the number of
   errors without new patch and with it, and if diff is more then 0 then
   patch are not taken.
  
   Looking a bit deeper it seems that Nova struggled with false positives
   and resorted to https://review.openstack.org/#/c/28754/ , which
 layers some
   historical checking of git on top of pylint's tendency to check only
 the
   latest commit. I can't say I'm too deeply versed in the code,  but
 it's
   enough to make me wonder if we want to go that direction and avoid the
   issues altogether?
 
  I haven’t looked at it in a while, but I’ve never been particularly
  excited by pylint. It’s extremely picky, encourages enforcing some
  questionable rules (arbitrary limits on variable name length?), and
 repots a
  lot of false positives. That combination tends to result in making
 writing
  software annoying without helping with quality in any real way.
 
  Doug
 
  
  
   I have taken as pattern Sahara's solution and proposed a patch for
   ceilometer:
   https://review.openstack.org/#/c/125906/
  
   Cheers,
   Igor Degtiarov
  
   ___
   OpenStack-dev mailing list
   OpenStack-dev@lists.openstack.org
   http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
  
  
  
  
   --
   Best regards,
   Dina Belova
   Software Engineer
   Mirantis Inc.
   ___
   OpenStack-dev mailing list
   OpenStack-dev@lists.openstack.org
   http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-06 Thread Igor Degtiarov
My points are next:

1. Pylint check will be very useful for project, and will help to
avoid critical errors or mistakes in code.

2. At first step we won't implement pylint as gate job, but will add
it at master to have a possibility to check  code with pylint locally,
if it is needed.

3. In future it could be added as a non-voting job.
-- Igor


On Sat, Oct 4, 2014 at 1:56 AM, Angus Lees g...@inodes.org wrote:
 You can turn off lots of the refactor recommendation checks.  I've been
 running pylint across neutron and it's uncovered half a dozen legitimate
 bugs so far - and that's with many tests still disabled.

 I agree that the defaults are too noisy, but its about the only tool that
 does linting across files - pep8 for example only looks at the current file
 (and not even the parse tree).

 On 4 Oct 2014 03:22, Doug Hellmann d...@doughellmann.com wrote:


 On Oct 3, 2014, at 1:09 PM, Neal, Phil phil.n...@hp.com wrote:

  From: Dina Belova [mailto:dbel...@mirantis.com]
  On Friday, October 03, 2014 2:53 AM
 
  Igor,
 
  Personally this idea looks really nice to me, as this will help to
  avoid
  strange code being merged and not found via reviewing process.
 
  Cheers,
  Dina
 
  On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov
  idegtia...@mirantis.com wrote:
  Hi folks!
 
  I try too guess do we need in ceilometer checking new patches for
  critical errors with pylint?
 
  As far as I know Nova and Sahara and others have such check. Actually
  it is not checking of all project but comparing of the number of
  errors without new patch and with it, and if diff is more then 0 then
  patch are not taken.
 
  Looking a bit deeper it seems that Nova struggled with false positives
  and resorted to https://review.openstack.org/#/c/28754/ , which layers some
  historical checking of git on top of pylint's tendency to check only the
  latest commit. I can't say I'm too deeply versed in the code,  but it's
  enough to make me wonder if we want to go that direction and avoid the
  issues altogether?

 I haven’t looked at it in a while, but I’ve never been particularly
 excited by pylint. It’s extremely picky, encourages enforcing some
 questionable rules (arbitrary limits on variable name length?), and repots a
 lot of false positives. That combination tends to result in making writing
 software annoying without helping with quality in any real way.

 Doug

 
 
  I have taken as pattern Sahara's solution and proposed a patch for
  ceilometer:
  https://review.openstack.org/#/c/125906/
 
  Cheers,
  Igor Degtiarov
 
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 
 
  --
  Best regards,
  Dina Belova
  Software Engineer
  Mirantis Inc.
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-03 Thread Dina Belova
Igor,

Personally this idea looks really nice to me, as this will help to avoid
strange code being merged and not found via reviewing process.

Cheers,
Dina

On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov idegtia...@mirantis.com
wrote:

 Hi folks!

 I try too guess do we need in ceilometer checking new patches for
 critical errors with pylint?

 As far as I know Nova and Sahara and others have such check. Actually
 it is not checking of all project but comparing of the number of
 errors without new patch and with it, and if diff is more then 0 then
 patch are not taken.

 I have taken as pattern Sahara's solution and proposed a patch for
 ceilometer:
 https://review.openstack.org/#/c/125906/

 Cheers,
 Igor Degtiarov

 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev




-- 

Best regards,

Dina Belova

Software Engineer

Mirantis Inc.
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-03 Thread Neal, Phil
 From: Dina Belova [mailto:dbel...@mirantis.com]
 On Friday, October 03, 2014 2:53 AM
 
 Igor,
 
 Personally this idea looks really nice to me, as this will help to avoid
 strange code being merged and not found via reviewing process.
 
 Cheers,
 Dina
 
 On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov
 idegtia...@mirantis.com wrote:
 Hi folks!
 
 I try too guess do we need in ceilometer checking new patches for
 critical errors with pylint?
 
 As far as I know Nova and Sahara and others have such check. Actually
 it is not checking of all project but comparing of the number of
 errors without new patch and with it, and if diff is more then 0 then
 patch are not taken.

Looking a bit deeper it seems that Nova struggled with false positives and 
resorted to https://review.openstack.org/#/c/28754/ , which layers some 
historical checking of git on top of pylint's tendency to check only the latest 
commit. I can't say I'm too deeply versed in the code,  but it's enough to make 
me wonder if we want to go that direction and avoid the issues altogether?

 
 I have taken as pattern Sahara's solution and proposed a patch for
 ceilometer:
 https://review.openstack.org/#/c/125906/
 
 Cheers,
 Igor Degtiarov
 
 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 
 
 --
 Best regards,
 Dina Belova
 Software Engineer
 Mirantis Inc.
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-03 Thread Doug Hellmann

On Oct 3, 2014, at 1:09 PM, Neal, Phil phil.n...@hp.com wrote:

 From: Dina Belova [mailto:dbel...@mirantis.com]
 On Friday, October 03, 2014 2:53 AM
 
 Igor,
 
 Personally this idea looks really nice to me, as this will help to avoid
 strange code being merged and not found via reviewing process.
 
 Cheers,
 Dina
 
 On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov
 idegtia...@mirantis.com wrote:
 Hi folks!
 
 I try too guess do we need in ceilometer checking new patches for
 critical errors with pylint?
 
 As far as I know Nova and Sahara and others have such check. Actually
 it is not checking of all project but comparing of the number of
 errors without new patch and with it, and if diff is more then 0 then
 patch are not taken.
 
 Looking a bit deeper it seems that Nova struggled with false positives and 
 resorted to https://review.openstack.org/#/c/28754/ , which layers some 
 historical checking of git on top of pylint's tendency to check only the 
 latest commit. I can't say I'm too deeply versed in the code,  but it's 
 enough to make me wonder if we want to go that direction and avoid the issues 
 altogether?

I haven’t looked at it in a while, but I’ve never been particularly excited by 
pylint. It’s extremely picky, encourages enforcing some questionable rules 
(arbitrary limits on variable name length?), and repots a lot of false 
positives. That combination tends to result in making writing software annoying 
without helping with quality in any real way.

Doug

 
 
 I have taken as pattern Sahara's solution and proposed a patch for
 ceilometer:
 https://review.openstack.org/#/c/125906/
 
 Cheers,
 Igor Degtiarov
 
 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 
 
 --
 Best regards,
 Dina Belova
 Software Engineer
 Mirantis Inc.
 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches

2014-10-03 Thread Angus Lees
You can turn off lots of the refactor recommendation checks.  I've been
running pylint across neutron and it's uncovered half a dozen legitimate
bugs so far - and that's with many tests still disabled.

I agree that the defaults are too noisy, but its about the only tool that
does linting across files - pep8 for example only looks at the current file
(and not even the parse tree).
On 4 Oct 2014 03:22, Doug Hellmann d...@doughellmann.com wrote:


 On Oct 3, 2014, at 1:09 PM, Neal, Phil phil.n...@hp.com wrote:

  From: Dina Belova [mailto:dbel...@mirantis.com]
  On Friday, October 03, 2014 2:53 AM
 
  Igor,
 
  Personally this idea looks really nice to me, as this will help to avoid
  strange code being merged and not found via reviewing process.
 
  Cheers,
  Dina
 
  On Fri, Oct 3, 2014 at 12:40 PM, Igor Degtiarov
  idegtia...@mirantis.com wrote:
  Hi folks!
 
  I try too guess do we need in ceilometer checking new patches for
  critical errors with pylint?
 
  As far as I know Nova and Sahara and others have such check. Actually
  it is not checking of all project but comparing of the number of
  errors without new patch and with it, and if diff is more then 0 then
  patch are not taken.
 
  Looking a bit deeper it seems that Nova struggled with false positives
 and resorted to https://review.openstack.org/#/c/28754/ , which layers
 some historical checking of git on top of pylint's tendency to check only
 the latest commit. I can't say I'm too deeply versed in the code,  but it's
 enough to make me wonder if we want to go that direction and avoid the
 issues altogether?

 I haven’t looked at it in a while, but I’ve never been particularly
 excited by pylint. It’s extremely picky, encourages enforcing some
 questionable rules (arbitrary limits on variable name length?), and repots
 a lot of false positives. That combination tends to result in making
 writing software annoying without helping with quality in any real way.

 Doug

 
 
  I have taken as pattern Sahara's solution and proposed a patch for
  ceilometer:
  https://review.openstack.org/#/c/125906/
 
  Cheers,
  Igor Degtiarov
 
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
 
 
 
 
  --
  Best regards,
  Dina Belova
  Software Engineer
  Mirantis Inc.
  ___
  OpenStack-dev mailing list
  OpenStack-dev@lists.openstack.org
  http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


 ___
 OpenStack-dev mailing list
 OpenStack-dev@lists.openstack.org
 http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev