11:19 a.m.
As we all know, Python is dynamically typed, which requires good
coverage by unit tests.
As we all know, Vdsm unit tests' coverage is not good.
We need all the help we can get in order to avoid typos leading to
bugs. Nir's `make pylint-diff` would give some. In order to have it
merged, I have posted
https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic...
to silence existing pylint errors. I hope I did not silence any
serious ones; please review this topic as soon as possible.
Regards,
Dan.
1:04 p.m.
On Fri, Apr 21, 2017 at 11:19 AM Dan Kenigsberg <danken(a)redhat.com> wrote:
As we all know, Python is dynamically typed, which requires good
coverage by unit tests.
As we all know, Vdsm unit tests' coverage is not good.
We need all the help we can get in order to avoid typos leading to
bugs. Nir's `make pylint-diff` would give some.
We can take this patch now, since it does not break the build on errors.
It is useful as is so people can run make pylint locally and fix their
errors.
In order to have it
merged, I have posted
https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic...
to silence existing pylint errors. I hope I did not silence any
serious ones; please review this topic as soon as possible.
Thanks for this great work!
I think the last patch is too big, and there are some storage issues that
we can fix
now. Can you split by vertical? I would like to take over the the storage
part.
I sent this patch making the storage/monitor.py pylint clean:
https://gerrit.ovirt.org/75736
Nir
1:28 p.m.
On Fri, Apr 21, 2017 at 1:04 PM, Nir Soffer <nsoffer(a)redhat.com> wrote:
On Fri, Apr 21, 2017 at 11:19 AM Dan Kenigsberg
<danken(a)redhat.com> wrote:
>
> As we all know, Python is dynamically typed, which requires good
> coverage by unit tests.
>
> As we all know, Vdsm unit tests' coverage is not good.
>
> We need all the help we can get in order to avoid typos leading to
> bugs. Nir's `make pylint-diff` would give some.
We can take this patch now, since it does not break the build on errors.
It is useful as is so people can run make pylint locally and fix their
errors.
>
> In order to have it
> merged, I have posted
>
>
https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic...
> to silence existing pylint errors. I hope I did not silence any
> serious ones; please review this topic as soon as possible.
Thanks for this great work!
I think the last patch is too big, and there are some storage issues that we
can fix
now. Can you split by vertical? I would like to take over the the storage
part.
Please do. Particularly, please look at the true error introduced by
I8e9bde2a0ad899ae56a18b4c7d6b405360d295f2 (there are two calls to
_deleteImage missing the new discard value).
I hope Piotr and Francesco can take over theirs, as the time I had to
invest in this is basically up (hence the gobbling of assorted fixes
in one ugly patch).
I sent this patch making the storage/monitor.py pylint clean:
https://gerrit.ovirt.org/75736
Nir
4:05 p.m.
On Fri, Apr 21, 2017 at 1:28 PM Dan Kenigsberg <danken(a)redhat.com> wrote:
On Fri, Apr 21, 2017 at 1:04 PM, Nir Soffer
<nsoffer(a)redhat.com> wrote:
> On Fri, Apr 21, 2017 at 11:19 AM Dan Kenigsberg <danken(a)redhat.com>
wrote:
>>
>> As we all know, Python is dynamically typed, which requires good
>> coverage by unit tests.
>>
>> As we all know, Vdsm unit tests' coverage is not good.
>>
>> We need all the help we can get in order to avoid typos leading to
>> bugs. Nir's `make pylint-diff` would give some.
>
>
> We can take this patch now, since it does not break the build on errors.
>
> It is useful as is so people can run make pylint locally and fix their
> errors.
>
>>
>> In order to have it
>> merged, I have posted
>>
>>
https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic...
>> to silence existing pylint errors. I hope I did not silence any
>> serious ones; please review this topic as soon as possible.
>
>
> Thanks for this great work!
>
> I think the last patch is too big, and there are some storage issues
that we
> can fix
> now. Can you split by vertical? I would like to take over the the storage
> part.
Please do.
I splitted the patches to:
- https://gerrit.ovirt.org/75728 pylint: brutally silence two
network-related errors
- https://gerrit.ovirt.org/75730 pylint: Silence pylint errors in gluster
- https://gerrit.ovirt.org/75748 pylint: Silence pylint errors in infra
- https://gerrit.ovirt.org/75749 pylint: Silence pylint errors in virt
- https://gerrit.ovirt.org/75750 pylint: Silence pylint errors in storage
I'll handle the storage topic, hopefully Sahina, Piotr, Francesco and Edy
will
care for the others.
Particularly, please look at the true error introduced by
I8e9bde2a0ad899ae56a18b4c7d6b405360d295f2 (there are two calls to
_deleteImage missing the new discard value).
We know about this, the code calling these is not used for years. The real
fix is to remove the unused code. If we will not have time to remove the
code
I'll leave the pylint comments.
I hope Piotr and Francesco can take over theirs, as the time I had to
invest in this is basically up (hence the gobbling of assorted fixes
in one ugly patch).
>
> I sent this patch making the storage/monitor.py pylint clean:
> https://gerrit.ovirt.org/75736
>
> Nir
3:54 a.m.
On Fri, Apr 21, 2017 at 4:05 PM Nir Soffer <nsoffer(a)redhat.com> wrote:
On Fri, Apr 21, 2017 at 1:28 PM Dan Kenigsberg
<danken(a)redhat.com> wrote:
> On Fri, Apr 21, 2017 at 1:04 PM, Nir Soffer <nsoffer(a)redhat.com> wrote:
> > On Fri, Apr 21, 2017 at 11:19 AM Dan Kenigsberg <danken(a)redhat.com>
> wrote:
> >>
> >> As we all know, Python is dynamically typed, which requires good
> >> coverage by unit tests.
> >>
> >> As we all know, Vdsm unit tests' coverage is not good.
> >>
> >> We need all the help we can get in order to avoid typos leading to
> >> bugs. Nir's `make pylint-diff` would give some.
> >
> >
> > We can take this patch now, since it does not break the build on errors.
> >
> > It is useful as is so people can run make pylint locally and fix their
> > errors.
> >
> >>
> >> In order to have it
> >> merged, I have posted
> >>
> >>
>
https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic...
> >> to silence existing pylint errors. I hope I did not silence any
> >> serious ones; please review this topic as soon as possible.
> >
> >
> > Thanks for this great work!
> >
> > I think the last patch is too big, and there are some storage issues
> that we
> > can fix
> > now. Can you split by vertical? I would like to take over the the
> storage
> > part.
>
> Please do.
I splitted the patches to:
- https://gerrit.ovirt.org/75728 pylint: brutally silence two
network-related errors
- https://gerrit.ovirt.org/75730 pylint: Silence pylint errors in gluster
- https://gerrit.ovirt.org/75748 pylint: Silence pylint errors in infra
- https://gerrit.ovirt.org/75749 pylint: Silence pylint errors in virt
- https://gerrit.ovirt.org/75750 pylint: Silence pylint errors in storage
The storage patch is replaced now by this topic:
https://gerrit.ovirt.org/#/q/topic:pylint-storage+is:open
pylint detected 3 real bugs:
- https://gerrit.ovirt.org/75786 pylint: Fix AttributeError hiding the real
error
- https://gerrit.ovirt.org/75787 pylint: Fix IscsiInterface.__getattr__
- https://gerrit.ovirt.org/75791 pylint: Fix AttributeError when renaming
non-empty directory
This patch fixes infra issues, since it caused many pylint errors in storage
modules. This will also fix the errors in utils.py:
- https://gerrit.ovirt.org/75792 pylint: Make utils.pidStat pylint friendly
One issue in sp.py cannot be improved now, so I silenced pylint errors:
- https://gerrit.ovirt.org/75803 pylint: Silence warning about
__securityOverride
The rest of the patches fix the code to make pylint happy and usually make
the code
easier to understand.
Once we merge this, storage will be pylint clean.
Please review,
Nir
I'll handle the storage topic, hopefully Sahina, Piotr, Francesco and Edy
will
care for the others.
Particularly, please look at the true error introduced by
> I8e9bde2a0ad899ae56a18b4c7d6b405360d295f2 (there are two calls to
> _deleteImage missing the new discard value).
>
We know about this, the code calling these is not used for years. The real
fix is to remove the unused code. If we will not have time to remove the
code
I'll leave the pylint comments.
I hope Piotr and Francesco can take over theirs, as the time I had to
> invest in this is basically up (hence the gobbling of assorted fixes
> in one ugly patch).
>
> >
> > I sent this patch making the storage/monitor.py pylint clean:
> > https://gerrit.ovirt.org/75736
> >
> > Nir
>
9:14 a.m.
This is a multi-part message in MIME format.
--------------4FAA4B405FADDEC36EE13185
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
On 04/23/2017 02:54 AM, Nir Soffer wrote:
On Fri, Apr 21, 2017 at 4:05 PM Nir Soffer <nsoffer(a)redhat.com
<mailto:nsoffer@redhat.com>> wrote:
> I think the last patch is too big, and there are some storage
issues that we
> can fix
> now. Can you split by vertical? I would like to take over
the the storage
> part.
Please do.
I splitted the patches to:
- https://gerrit.ovirt.org/75728 pylint: brutally silence two
network-related errors
- https://gerrit.ovirt.org/75730 pylint: Silence pylint errors in
gluster
- https://gerrit.ovirt.org/75748 pylint: Silence pylint errors in
infra
- https://gerrit.ovirt.org/75749 pylint: Silence pylint errors in virt
I'm taking care of this today
Thanks for kickstarting this!
--
Francesco Romani
Senior SW Eng., Virtualization R&D
Red Hat
IRC: fromani github: @fromanirh
--------------4FAA4B405FADDEC36EE13185
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 8bit
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 04/23/2017 02:54 AM, Nir Soffer wrote:<br>
<blockquote type="cite"
cite="mid:CAMRbyyuoyW9zS8OsLw8LVPn8LA-K252jh5u9zbxXqe2aBUWmwQ@mail.gmail.com">
<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"
moz-do-not-send="true">nsoffer(a)redhat.com</a>&gt;
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">> I think the last patch is
too big, and there are some storage issues that we<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
> 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"
moz-do-not-send="true">https://gerrit.ovirt.org/75728</a&...:
brutally silence two network-related errors</div>
<div>- <a href="https://gerrit.ovirt.org/75730"
target="_blank"
moz-do-not-send="true">https://gerrit.ovirt.org/75730</a&...:
Silence pylint errors in gluster</div>
<div>- <a href="https://gerrit.ovirt.org/75748"
target="_blank"
moz-do-not-send="true">https://gerrit.ovirt.org/75748</a&...:
Silence pylint errors in infra</div>
<div>- <a href="https://gerrit.ovirt.org/75749"
target="_blank"
moz-do-not-send="true">https://gerrit.ovirt.org/75749</a&...:
Silence pylint errors in virt</div>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
I'm taking care of this today<br>
<br>
Thanks for kickstarting this!<br>
<br>
<pre class="moz-signature" cols="72">--
Francesco Romani
Senior SW Eng., Virtualization R&D
Red Hat
IRC: fromani github: @fromanirh</pre>
</body>
</html>
--------------4FAA4B405FADDEC36EE13185--
2769
days inactive
2772
days old
5 comments
3 participants
participants (3)
-
Dan Kenigsberg -
Francesco Romani -
Nir Soffer