[blivet][master] pylint-inspired fixes

[Vratislav Podzimek]

These things were reported by pylint when I tried to clean everything up before
starting to port blivet to libblockdev. One issue still remains:

************* Module blivet/formats/fs.py
blivet/formats/fs.py:1499: [W0201(attribute-defined-outside-init), NoDevFS._setDevice] Attribute '_device' defined outside __init__

but I don't want to create a conflict with mulhern's patches for the NoDevFS class.

Vratislav Podzimek (3):
  Disable pylint check for cached LVM data in more places
  Define the _device attribute in constructor
  Do not create lambda over and over in a cycle

blivet/devicetree.py | 4 ++--
 blivet/formats/fs.py | 4 ++++
 scripts/makebumpver  | 3 ++-
 3 files changed, 8 insertions(+), 3 deletions(-)

[PATCH 1/3] Disable pylint check for cached LVM data in more places

[Vratislav Podzimek]

We disable the check few lines below so we should do the same in this place.

Signed-off-by: Vratislav Podzimek 
---
 blivet/devicetree.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/blivet/devicetree.py b/blivet/devicetree.py
index 166bf99..687897e 100644
--- a/blivet/devicetree.py
+++ b/blivet/devicetree.py
@@ -172,14 +172,14 @@ class DeviceTree(object):
     @property
     def pvInfo(self):
         if self._pvInfo is None:
-            self._pvInfo = lvm.pvinfo()
+            self._pvInfo = lvm.pvinfo() # pylint: disable=attribute-defined-outside-init

return self._pvInfo

@property
     def lvInfo(self):
         if self._lvInfo is None:
-            self._lvInfo = lvm.lvs()
+            self._lvInfo = lvm.lvs() # pylint: disable=attribute-defined-outside-init

return self._lvInfo

[PATCH 2/3] Define the _device attribute in constructor

[Vratislav Podzimek]

That's how good guys do it.

Signed-off-by: Vratislav Podzimek 
---
 blivet/formats/fs.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
index ad2542a..4bee167 100644
--- a/blivet/formats/fs.py
+++ b/blivet/formats/fs.py
@@ -1443,6 +1443,10 @@ class NFS(FS):
     _type = "nfs"
     _modules = ["nfs"]

+    def __init__(self, *args, **kwargs):
+        super(NFS, self).__init__(*args, **kwargs)
+        self._device = None
+
     def _deviceCheck(self, devspec):
         if devspec is not None and ":" not in devspec:
             raise ValueError("device must be of the form <host>:<path>")

[PATCH 2/3] Define the _device attribute in constructor

[Anne Mulhern]

----- Original Message -----
> From: "Vratislav Podzimek" 
> To: [e-mail]
> Sent: Wednesday, January 21, 2015 5:50:35 AM
> Subject: [PATCH 2/3] Define the _device attribute in constructor
>
> That's how good guys do it.
> Signed-off-by: Vratislav Podzimek
>
> blivet/formats/fs.py | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> index ad2542a..4bee167 100644
> --- a/blivet/formats/fs.py
> +++ b/blivet/formats/fs.py
> @@ -1443,6 +1443,10 @@ class NFS(FS):
>      _type = "nfs"
>      _modules = ["nfs"]
> def __init__(self, *args, **kwargs):
>    super(NFS, self).__init__(*args, **kwargs)
>    self._device = None
> def _deviceCheck(self, devspec):
>        if devspec is not None and ":" not in devspec:
>            raise ValueError("device must be of the form <host>:<path>")
>
> --
> 2.1.0
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

I believe that:

Superclass ctr is automatically invoked if there is no ctr defined,
and DeviceFormat.__init__() sets self._device to None if no device passed
via constructor.

So, the only reason for this code to exist would be to enforce that NFS filesystem
format should always have a device of None, even if a device is passed.

I don't see why that should be the case for NFS.

- mulhern

[PATCH 2/3] Define the _device attribute in constructor

[Vratislav Podzimek]

On Thu, 2015-01-22 at 09:24 -0500, Anne Mulhern wrote:
> ----- Original Message -----
> > From: "Vratislav Podzimek" 
> > To: [e-mail]
> > Sent: Wednesday, January 21, 2015 5:50:35 AM
> > Subject: [PATCH 2/3] Define the _device attribute in constructor
> >
> > That's how good guys do it.
> > Signed-off-by: Vratislav Podzimek
> >
> > blivet/formats/fs.py | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/blivet/formats/fs.py b/blivet/formats/fs.py
> > index ad2542a..4bee167 100644
> > --- a/blivet/formats/fs.py
> > +++ b/blivet/formats/fs.py
> > @@ -1443,6 +1443,10 @@ class NFS(FS):
> >      _type = "nfs"
> >      _modules = ["nfs"]
> > def __init__(self, *args, **kwargs):
> >    super(NFS, self).__init__(*args, **kwargs)
> >    self._device = None
> > def _deviceCheck(self, devspec):
> >        if devspec is not None and ":" not in devspec:
> >            raise ValueError("device must be of the form <host>:<path>")
> >
> > --
> > 2.1.0
> >
> > anaconda-patches mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
> I believe that:
>
> Superclass ctr is automatically invoked if there is no ctr defined,
> and DeviceFormat.__init__() sets self._device to None if no device passed
> via constructor.
>
> So, the only reason for this code to exist would be to enforce that NFS filesystem
> format should always have a device of None, even if a device is passed.
>
> I don't see why that should be the case for NFS.

I see that my yesterday's reply got lost somewhere, but it was something
like:

Good catch, you're right. The issue is that the self._device attribute
is not defined in DeviceFormat.__init__, it's only set via the
self.device property which has the '# pylint: disable'. However, that
only covers that particular case. Sending a new patch instead of this
one that fixes the issue properly.

[PATCH 2/3 v2] Define the _device, _label and _options attributes in constructor

[Vratislav Podzimek]

That's how good guys do it.

Signed-off-by: Vratislav Podzimek 
---
 blivet/formats/__init__.py | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/blivet/formats/__init__.py b/blivet/formats/__init__.py
index 907eb20..5e97620 100644
--- a/blivet/formats/__init__.py
+++ b/blivet/formats/__init__.py
@@ -179,6 +179,10 @@ class DeviceFormat(ObjectID):
                 it via the 'device' kwarg to the :meth:`create` method.
         """
         ObjectID.__init__(self)
+        self._label = None
+        self._options = None
+        self._device = None
+
         self.device = kwargs.get("device")
         self.uuid = kwargs.get("uuid")
         self.exists = kwargs.get("exists", False)
@@ -271,7 +275,7 @@ class DeviceFormat(ObjectID):

This method is not intended to be overridden.
         """
-        self._label = label # pylint: disable=attribute-defined-outside-init
+        self._label = label

def _getLabel(self):
         """The label for this filesystem.
@@ -284,7 +288,7 @@ class DeviceFormat(ObjectID):
         return self._label

def _setOptions(self, options):
-        self._options = options # pylint: disable=attribute-defined-outside-init
+        self._options = options

def _getOptions(self):
         return self._options
@@ -294,7 +298,7 @@ class DeviceFormat(ObjectID):
     def _setDevice(self, devspec):
         if devspec and not devspec.startswith("/"):
             raise ValueError("device must be a fully qualified path")
-        self._device = devspec # pylint: disable=attribute-defined-outside-init
+        self._device = devspec

def _getDevice(self):
         return self._device

[PATCH 2/3 v2] Define the _device, _label and _options attributes in constructor

[Anne Mulhern]

These look fine to me.

- mulhern

----- Original Message -----
> From: "Vratislav Podzimek" 
> To: [e-mail]
> Sent: Friday, January 23, 2015 3:34:22 AM
> Subject: [PATCH 2/3 v2] Define the _device,	_label and _options attributes in constructor
>
> That's how good guys do it.
> Signed-off-by: Vratislav Podzimek
>
> blivet/formats/__init__.py | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/blivet/formats/__init__.py b/blivet/formats/__init__.py
> index 907eb20..5e97620 100644
> --- a/blivet/formats/__init__.py
> +++ b/blivet/formats/__init__.py
> @@ -179,6 +179,10 @@ class DeviceFormat(ObjectID):
>                  it via the 'device' kwarg to the :meth:`create` method.
>          """
>          ObjectID.__init__(self)
>    self._label = None
>    self._options = None
>    self._device = None
>    self.device = kwargs.get("device")
>    self.uuid = kwargs.get("uuid")
>    self.exists = kwargs.get("exists", False)
>
> @@ -271,7 +275,7 @@ class DeviceFormat(ObjectID):
>         This method is not intended to be overridden.
>      """
>    self._label = label # pylint: disable=attribute-defined-outside-init
>    self._label = label
>
> def _getLabel(self):
>        """The label for this filesystem.
>
> @@ -284,7 +288,7 @@ class DeviceFormat(ObjectID):
>          return self._label
>  def _setOptions(self, options):
>    self._options = options # pylint:
>
> disable=attribute-defined-outside-init
>    self._options = options
>
> def _getOptions(self):
>        return self._options
>
> @@ -294,7 +298,7 @@ class DeviceFormat(ObjectID):
>      def _setDevice(self, devspec):
>          if devspec and not devspec.startswith("/"):
>              raise ValueError("device must be a fully qualified path")
>    self._device = devspec # pylint:
>
> disable=attribute-defined-outside-init
>    self._device = devspec
>
> def _getDevice(self):
>        return self._device
>
> --
> 2.1.0
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

[PATCH 3/3] Do not create lambda over and over in a cycle

[Vratislav Podzimek]

Let's just define and create it once and use it over and over.

Signed-off-by: Vratislav Podzimek 
---
 scripts/makebumpver | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/makebumpver b/scripts/makebumpver
index 57f549f..07ce065 100755
--- a/scripts/makebumpver
+++ b/scripts/makebumpver
@@ -240,8 +240,9 @@ class MakeBumpVer:
                        proc[0].strip('\n').split('\n'))

if self.ignore and self.ignore != '':
+            startswith_commit = lambda x: not x.startswith(commit)
             for commit in self.ignore.split(','):
-                lines = filter(lambda x: not x.startswith(commit), lines)
+                lines = filter(startswith_commit, lines)

rpm_log = []
         bad_bump = False

[PATCH 3/3] Do not create lambda over and over in a cycle

[Anne Mulhern]

----- Original Message -----
> From: "Vratislav Podzimek" 
> To: [e-mail]
> Sent: Wednesday, January 21, 2015 5:50:36 AM
> Subject: [PATCH 3/3] Do not create lambda over and over in a cycle
>
> Let's just define and create it once and use it over and over.
> Signed-off-by: Vratislav Podzimek
>
> scripts/makebumpver | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/makebumpver b/scripts/makebumpver
> index 57f549f..07ce065 100755
> --- a/scripts/makebumpver
> +++ b/scripts/makebumpver
> @@ -240,8 +240,9 @@ class MakeBumpVer:
>                         proc[0].strip('\n').split('\n'))
>      if self.ignore and self.ignore != '':
>        startswith_commit = lambda x: not x.startswith(commit)
>        for commit in self.ignore.split(','):
>            lines = filter(lambda x: not x.startswith(commit), lines)
>            lines = filter(startswith_commit, lines)
>
>    rpm_log = []
>    bad_bump = False
>
> --
> 2.1.0
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches

List comprehension would be that much prettier.

lines = [l for l in lines if not x.startswith(commit)]

or even,

commits = self.ignore.split(',')
lines = [l for l in lines if not any(l.startswith(c) for c in commits)]

which has advantage of not recomputing new list for every element in commit.

- mulhern

[PATCH 3/3] Do not create lambda over and over in a cycle

[Vratislav Podzimek]

On Thu, 2015-01-22 at 09:40 -0500, Anne Mulhern wrote:
> ----- Original Message -----
> > From: "Vratislav Podzimek" 
> > To: [e-mail]
> > Sent: Wednesday, January 21, 2015 5:50:36 AM
> > Subject: [PATCH 3/3] Do not create lambda over and over in a cycle
> >
> > Let's just define and create it once and use it over and over.
> > Signed-off-by: Vratislav Podzimek
> >
> > scripts/makebumpver | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/makebumpver b/scripts/makebumpver
> > index 57f549f..07ce065 100755
> > --- a/scripts/makebumpver
> > +++ b/scripts/makebumpver
> > @@ -240,8 +240,9 @@ class MakeBumpVer:
> >                         proc[0].strip('\n').split('\n'))
> >      if self.ignore and self.ignore != '':
> >        startswith_commit = lambda x: not x.startswith(commit)
> >        for commit in self.ignore.split(','):
> >            lines = filter(lambda x: not x.startswith(commit), lines)
> >            lines = filter(startswith_commit, lines)
> >
> >    rpm_log = []
> >    bad_bump = False
> >
> > --
> > 2.1.0
> >
> > anaconda-patches mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
>
> List comprehension would be that much prettier.
>
> lines = [l for l in lines if not x.startswith(commit)]
>
> or even,
>
> commits = self.ignore.split(',')
> lines = [l for l in lines if not any(l.startswith(c) for c in commits)]
>
> which has advantage of not recomputing new list for every element in commit.

Agreed, but I'd still rather have two separate commits like:
* make pylint shut up
* make our code prettier

so unless somebody strongly disagrees, I'd like to push my original
patch and then add your suggested change as a follow-up commit.

[Samantha N. Bueno]

On Wed, Jan 21, 2015 at 11:50:33AM +0100, Vratislav Podzimek wrote:
> These things were reported by pylint when I tried to clean everything up before
> starting to port blivet to libblockdev. One issue still remains:
>
> ************* Module blivet/formats/fs.py
> blivet/formats/fs.py:1499: [W0201(attribute-defined-outside-init), NoDevFS._setDevice] Attribute '_device' defined outside __init__
>
> but I don't want to create a conflict with mulhern's patches for the NoDevFS class.
>
> Vratislav Podzimek (3):
>   Disable pylint check for cached LVM data in more places
>   Define the _device attribute in constructor
>   Do not create lambda over and over in a cycle

These all three look ok to me.
> blivet/devicetree.py | 4 ++--
>  blivet/formats/fs.py | 4 ++++
>  scripts/makebumpver  | 3 ++-
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> -- 
> 2.1.0
>
> anaconda-patches mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches