Hi Bruno,
On 4/17/24 11:02 AM, Bruno Haible wrote:
>> $ pylint *.py | grep W0201
>> GLError.py:119:12: W0201: Attribute 'message' defined outside __init__
>> (attribute-defined-outside-init)
>
> This warning is bogus. In a dynamic language like Python, it is
> perfectly fine to have a property used only by one method. Only if
> it is used by more than one method, would I find it suitable to
> declare/initialize it in the constructor.
I respectfully disagree about the warning message being bogus, but I
think I generally agree beyond that. Though, when I see an instance
variable used only in one function it makes me think that it should be
local to the function it is used in.
The reason I dislike defining instance variables outside of __init__
is that you can run into situations like this, which I find hard to
follow:
var = GLImport(...) # self.assistant does not exist.
var.assistant.do_something() # This is invalid.
var.execute(...) # self.assistant is now defined.
var.assistant.do_something() # Now this is valid.
I think it is best to avoid this, but there are probably situations
where it may be intentional/necessary.
> Your patch is not good, because it increases the code that is
> necessary to read, in order to understand the property: before,
> it was one method, now it would be the class.
>
> Maybe you can silence the warning by prepending a '_' to the
> property name? That would be acceptable.
The issue with the 'self.messages' in GLError is that it appears that
__repr__ expects 'self.messages' to be initialized to None in
__init__. I think the idea was to have it so that under
'if __name__ == "__main__"':
try:
main()
except GLError as exc:
sys.stderr.write(repr(exc))
instead of rewriting the messages there. I didn't write it though so maybe
I am misreading. :)
Let's leave that until I can properly fix that class.
>> GLImport.py:1043:8: W0201: Attribute 'assistant' defined outside __init__
>> (attribute-defined-outside-init)
>
> Similarly. In the current state, this property could be turned into a
> local variable. If you need this attribute in other methods (see the
> other thread), please find a good place to initialize it. But the
> proposed patch here is not good, as it initializes the same property
> twice, and who knows if both initializations are really equivalent?
Good point. I was under the impression that GLConfig does not get
modified after being passed from main(). Though I should check and
document it before making changes with that assumption.
Patch 0002 makes this GLFileAssistant local to GLImport.execute().
Since it is only used there I agree that it makes more sense.
>> We already unnecessarily create a
>> GLFileSystem that we don't need to [2] [3]. :)
>
> You're welcome to remove the unneeded property.
In patch 0001 attached.
CollinFrom e1c70606ae64faee9ced3ebc4c46e9c86a689a58 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Wed, 17 Apr 2024 11:34:07 -0700
Subject: [PATCH 1/2] gnulib-tool.py: Remove an unused instance attribute.
* pygnulib/GLImport.py (GLImport.__init__): Remove the unused
GLFileSystem object.
---
ChangeLog | 6 ++++++
pygnulib/GLImport.py | 1 -
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/ChangeLog b/ChangeLog
index adf89411d2..dbe6edd52d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-04-17 Collin Funk <collin.fu...@gmail.com>
+
+ gnulib-tool.py: Remove an unused instance attribute.
+ * pygnulib/GLImport.py (GLImport.__init__): Remove the unused
+ GLFileSystem object.
+
2024-04-17 Collin Funk <collin.fu...@gmail.com>
gnulib-tool.py: Fix a pylint 'attribute-defined-outside-init' warning.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index c6a4693c90..0933943d4c 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -302,7 +302,6 @@ def __init__(self, config: GLConfig, mode: int) -> None:
# Define GLImport attributes.
self.emitter = GLEmiter(self.config)
- self.filesystem = GLFileSystem(self.config)
self.modulesystem = GLModuleSystem(self.config)
self.moduletable = GLModuleTable(self.config,
self.config.checkInclTestCategory(TESTS['all-tests']),
--
2.44.0
From d3306868fdd69724442c05b1b1526bc9c279bd65 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Wed, 17 Apr 2024 11:47:16 -0700
Subject: [PATCH 2/2] gnulib-tool.py: Make an instance variable local to a
function.
* pygnulib/GLImport.py (GLImport.execute): Define the GLFileAssistant as
local to this function because it is unused elsewhere.
---
ChangeLog | 6 ++++++
pygnulib/GLImport.py | 48 ++++++++++++++++++++++----------------------
2 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index dbe6edd52d..348c887be0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-04-17 Collin Funk <collin.fu...@gmail.com>
+
+ gnulib-tool.py: Make an instance variable local to a function.
+ * pygnulib/GLImport.py (GLImport.execute): Define the GLFileAssistant as
+ local to this function because it is unused elsewhere.
+
2024-04-17 Collin Funk <collin.fu...@gmail.com>
gnulib-tool.py: Remove an unused instance attribute.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index 0933943d4c..0a19d50e00 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -1039,7 +1039,7 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
print('Create directory %s' % directory)
# Create GLFileAssistant instance to process files.
- self.assistant = GLFileAssistant(self.config, transformers)
+ assistant = GLFileAssistant(self.config, transformers)
# Files which are in filetable['old'] and not in filetable['new'].
# They will be removed and added to filetable['removed'] list.
@@ -1074,9 +1074,9 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
for pair in pairs:
original = pair[1]
rewritten = pair[0]
- self.assistant.setOriginal(original)
- self.assistant.setRewritten(rewritten)
- self.assistant.add_or_update(already_present)
+ assistant.setOriginal(original)
+ assistant.setRewritten(rewritten)
+ assistant.add_or_update(already_present)
# Files which are in filetable['new'] and in filetable['old'].
# They will be added/updated and added to filetable['added'] list.
@@ -1088,12 +1088,12 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
for pair in pairs:
original = pair[1]
rewritten = pair[0]
- self.assistant.setOriginal(original)
- self.assistant.setRewritten(rewritten)
- self.assistant.add_or_update(already_present)
+ assistant.setOriginal(original)
+ assistant.setRewritten(rewritten)
+ assistant.add_or_update(already_present)
# Add files which were added to the list of filetable['added'].
- filetable['added'] += self.assistant.getFiles()
+ filetable['added'] += assistant.getFiles()
filetable['added'] = sorted(set(filetable['added']))
# Default the source makefile name to Makefile.am.
@@ -1134,12 +1134,12 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
if pobase:
# Create po makefile and auxiliary files.
for file in ['Makefile.in.in', 'remove-potcdate.sin']:
- tmpfile = self.assistant.tmpfilename(joinpath(pobase, file))
+ tmpfile = assistant.tmpfilename(joinpath(pobase, file))
path = joinpath('build-aux', 'po', file)
lookedup, flag = filesystem.lookup(path)
copyfile(lookedup, tmpfile)
basename = joinpath(pobase, file)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
@@ -1156,11 +1156,11 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Create po makefile parameterization, part 1.
basename = joinpath(pobase, 'Makevars')
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
emit = self.emitter.po_Makevars()
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(emit)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
@@ -1177,11 +1177,11 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Create po makefile parameterization, part 2.
basename = joinpath(pobase, 'POTFILES.in')
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(self.emitter.po_POTFILES_in(filetable['all']))
basename = joinpath(pobase, 'POTFILES.in')
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
@@ -1211,7 +1211,7 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Create po/LINGUAS.
basename = joinpath(pobase, 'LINGUAS')
if not self.config['dryrun']:
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
data = '# Set of available languages.\n'
files = sorted([ constants.subend('.po', '', file)
for file in os.listdir(joinpath(destdir, pobase))
@@ -1219,7 +1219,7 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
data += lines_to_multiline(files)
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(data)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
print('Updating %s (backup in %s)' % (filename, backup))
elif flag == 2:
@@ -1236,11 +1236,11 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Create m4/gnulib-cache.m4.
basename = joinpath(m4base, 'gnulib-cache.m4')
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
emit = self.gnulib_cache()
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(emit)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
@@ -1261,11 +1261,11 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Create m4/gnulib-comp.m4.
basename = joinpath(m4base, 'gnulib-comp.m4')
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
emit = self.gnulib_comp(filetable, gentests)
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(emit)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
@@ -1288,7 +1288,7 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Do this after creating gnulib-comp.m4, because func_emit_lib_Makefile_am
# can run 'autoconf -t', which reads gnulib-comp.m4.
basename = joinpath(sourcebase, source_makefile_am)
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
emit = self.emitter.lib_Makefile_am(basename, self.moduletable.getMainModules(),
self.moduletable, self.makefiletable,
actioncmd, for_test)
@@ -1298,7 +1298,7 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
input=emit, text=True, capture_output=True).stdout
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(emit)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
@@ -1316,13 +1316,13 @@ def execute(self, filetable: dict[str, list[str]], transformers: dict[str, str])
# Create tests Makefile.
if gentests:
basename = joinpath(testsbase, tests_makefile_am)
- tmpfile = self.assistant.tmpfilename(basename)
+ tmpfile = assistant.tmpfilename(basename)
emit = self.emitter.tests_Makefile_am(basename, self.moduletable.getTestsModules(),
self.moduletable, self.makefiletable,
'%stests_WITNESS' % macro_prefix, for_test)
with open(tmpfile, mode='w', newline='\n', encoding='utf-8') as file:
file.write(emit)
- filename, backup, flag = self.assistant.super_update(basename, tmpfile)
+ filename, backup, flag = assistant.super_update(basename, tmpfile)
if flag == 1:
if not self.config['dryrun']:
print('Updating %s (backup in %s)' % (filename, backup))
--
2.44.0
