Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches
>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 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 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" wrote: > >> > >> > >> On Oct 3, 2014, at 1:09 PM, Neal, Phil 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 > >> >> 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
> 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 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 > > 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
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 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" wrote: >> >> >> On Oct 3, 2014, at 1:09 PM, Neal, Phil 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 >> >> 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
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" wrote: > > On Oct 3, 2014, at 1:09 PM, Neal, Phil 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 > >> 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
Re: [openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches
On Oct 3, 2014, at 1:09 PM, Neal, Phil 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 >> 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
> 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 > 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
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 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
[openstack-dev] [Ceilometer] Adding pylint checking of new ceilometer patches
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
