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(-)
We disable the check few lines below so we should do the same in this place.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- 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
That's how good guys do it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- 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>")
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org 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 vpodzime@redhat.com
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 anaconda-patches@lists.fedorahosted.org 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
On Thu, 2015-01-22 at 09:24 -0500, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org 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 vpodzime@redhat.com
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 anaconda-patches@lists.fedorahosted.org 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.
That's how good guys do it.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- 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
These look fine to me.
- mulhern
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org 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 vpodzime@redhat.com
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 = Noneself._options = Noneself._device = Noneself.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 = labeldef _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 = optionsdef _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 = devspecdef _getDevice(self): return self._device
-- 2.1.0
anaconda-patches mailing list anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
Let's just define and create it once and use it over and over.
Signed-off-by: Vratislav Podzimek vpodzime@redhat.com --- 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
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org 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 vpodzime@redhat.com
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 anaconda-patches@lists.fedorahosted.org 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
On Thu, 2015-01-22 at 09:40 -0500, Anne Mulhern wrote:
----- Original Message -----
From: "Vratislav Podzimek" vpodzime@redhat.com To: anaconda-patches@lists.fedorahosted.org 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 vpodzime@redhat.com
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 anaconda-patches@lists.fedorahosted.org 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.
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 anaconda-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches
anaconda-patches@lists.fedorahosted.org