<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Apr 21, 2017 at 4:05 PM Nir Soffer <<a href="mailto:nsoffer@redhat.com">nsoffer@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Apr 21, 2017 at 1:28 PM Dan Kenigsberg <<a href="mailto:danken@redhat.com" target="_blank">danken@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Apr 21, 2017 at 1:04 PM, Nir Soffer <<a href="mailto:nsoffer@redhat.com" target="_blank">nsoffer@redhat.com</a>> wrote:<br>
> On Fri, Apr 21, 2017 at 11:19 AM Dan Kenigsberg <<a href="mailto:danken@redhat.com" target="_blank">danken@redhat.com</a>> wrote:<br>
>><br>
>> As we all know, Python is dynamically typed, which requires good<br>
>> coverage by unit tests.<br>
>><br>
>> As we all know, Vdsm unit tests' coverage is not good.<br>
>><br>
>> We need all the help we can get in order to avoid typos leading to<br>
>> bugs. Nir's `make pylint-diff` would give some.<br>
><br>
><br>
> We can take this patch now, since it does not break the build on errors.<br>
><br>
> It is useful as is so people can run make pylint locally and fix their<br>
> errors.<br>
><br>
>><br>
>> In order to have it<br>
>> merged, I have posted<br>
>><br>
>> <a href="https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:pylint+owner:danken" rel="noreferrer" target="_blank">https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:pylint+owner:danken</a><br>
>> to silence existing pylint errors. I hope I did not silence any<br>
>> serious ones; please review this topic as soon as possible.<br>
><br>
><br>
> Thanks for this great work!<br>
><br>
> I think the last patch is too big, and there are some storage issues that we<br>
> can fix<br>
> now. Can you split by vertical? I would like to take over the the storage<br>
> part.<br>
<br>
Please do. </blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>I splitted the patches to:</div><div><br></div><div>- <a href="https://gerrit.ovirt.org/75728" target="_blank">https://gerrit.ovirt.org/75728</a> pylint: brutally silence two network-related errors</div><div>- <a href="https://gerrit.ovirt.org/75730" target="_blank">https://gerrit.ovirt.org/75730</a> pylint: Silence pylint errors in gluster</div><div>- <a href="https://gerrit.ovirt.org/75748" target="_blank">https://gerrit.ovirt.org/75748</a> pylint: Silence pylint errors in infra</div><div>- <a href="https://gerrit.ovirt.org/75749" target="_blank">https://gerrit.ovirt.org/75749</a> pylint: Silence pylint errors in virt</div><div>- <a href="https://gerrit.ovirt.org/75750" target="_blank">https://gerrit.ovirt.org/75750</a> pylint: Silence pylint errors in storage<br></div></div></div></blockquote><div><br></div><div>The storage patch is replaced now by this topic:</div><div><a href="https://gerrit.ovirt.org/#/q/topic:pylint-storage+is:open">https://gerrit.ovirt.org/#/q/topic:pylint-storage+is:open</a><br></div><div><br></div><div>pylint detected 3 real bugs:</div><div>- <a href="https://gerrit.ovirt.org/75786">https://gerrit.ovirt.org/75786</a> pylint: Fix AttributeError hiding the real error<br></div><div>- <a href="https://gerrit.ovirt.org/75787">https://gerrit.ovirt.org/75787</a> pylint: Fix IscsiInterface.__getattr__</div><div>- <a href="https://gerrit.ovirt.org/75791">https://gerrit.ovirt.org/75791</a> pylint: Fix AttributeError when renaming non-empty directory</div><div><br></div><div>This patch fixes infra issues, since it caused many pylint errors in storage</div><div>modules. This will also fix the errors in utils.py:</div><div>- <a href="https://gerrit.ovirt.org/75792">https://gerrit.ovirt.org/75792</a> pylint: Make utils.pidStat pylint friendly</div><div><br></div><div>One issue in sp.py cannot be improved now, so I silenced pylint errors:</div><div>- <a href="https://gerrit.ovirt.org/75803">https://gerrit.ovirt.org/75803</a> pylint: Silence warning about __securityOverride<br></div><div><br></div><div><div>The rest of the patches fix the code to make pylint happy and usually make the code</div><div>easier to understand.</div><br class="inbox-inbox-Apple-interchange-newline"></div><div>Once we merge this, storage will be pylint clean.</div><div><br></div><div>Please review,</div><div>Nir</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"> <br><br><br><br><div>I'll handle the storage topic, hopefully Sahina, Piotr, Francesco and Edy will </div><div>care for the others.</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Particularly, please look at the true error introduced by<br>
I8e9bde2a0ad899ae56a18b4c7d6b405360d295f2 (there are two calls to<br>
_deleteImage missing the new discard value).<br></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>We know about this, the code calling these is not used for years. The real</div><div>fix is to remove the unused code. If we will not have time to remove the code</div><div>I'll leave the pylint comments.</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I hope Piotr and Francesco can take over theirs, as the time I had to<br>
invest in this is basically up (hence the gobbling of assorted fixes<br>
in one ugly patch).<br>
<br>
><br>
> I sent this patch making the storage/monitor.py pylint clean:<br>
> <a href="https://gerrit.ovirt.org/75736" rel="noreferrer" target="_blank">https://gerrit.ovirt.org/75736</a><br>
><br>
> Nir<br>
</blockquote></div></div></blockquote></div></div>