Re: gnulib-tool.py: Fix pylint 'attribute-defined-outside-init' warnings.

[Collin Funk]

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.

Collin

0001-gnulib-tool.py-Remove-an-unused-instance-attribute.patch
Description: Text Data

0002-gnulib-tool.py-Make-an-instance-variable-local-to-a-.patch
Description: Text Data