gnulib-tool.py: Omit some unnecessary list() calls around sorted().

gnulib-tool.py: Omit some unnecessary list() calls around sorted().

[Collin Funk]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

Here is two janitorial work patches.

I remember an extra dict() call causing trouble with GLMakefileTable.
Removing the extra list() calls from sorted() seemed like a good place
to start cleaning up.

The second patch removes this function:

    def removeFile(self, file: str) -> None:
        '''Remove file from the list of added files.'''
        if file in self.added:
            self.added.pop(file)

This is incorrect since list.pop() takes an index and not a list
element. This function is unused so I've chosen to remove it. If it is
ever needed in the future the correct method would be list.remove().

Collin

[-- Attachment #2: 0001-gnulib-tool.py-Omit-some-unnecessary-list-calls-arou.patch --]
[-- Type: text/x-patch, Size: 5127 bytes --]

From d5190037c894a12eb7cbaaee1b89800cbb56615a Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 7 Apr 2024 18:49:24 -0700
Subject: [PATCH 1/2] gnulib-tool.py: Omit some unnecessary list() calls around
 sorted().

* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am): Remove the list()
call in the argument to sorted. The sorted() function works on any
iterable and always returns a list.
* pygnulib/GLImport.py (GLImport.rewrite_old_files)
(GLImport.rewrite_new_files): Likewise.
* pygnulib/GLModuleSystem.py (GLModuleTable.transitive_closure)
(GLModuleTable.transitive_closure_separately): Likewise.
* pygnulib/GLTestDir.py (GLTestDir.rewrite_files): Likewise.
---
 ChangeLog                  | 12 ++++++++++++
 pygnulib/GLEmiter.py       |  4 ++--
 pygnulib/GLImport.py       |  6 ++----
 pygnulib/GLModuleSystem.py |  4 ++--
 pygnulib/GLTestDir.py      |  3 +--
 5 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 535dfc04cc..4fd962ecbb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2024-04-07  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Omit some unnecessary list() calls around sorted().
+	* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am): Remove the list()
+	call in the argument to sorted. The sorted() function works on any
+	iterable and always returns a list.
+	* pygnulib/GLImport.py (GLImport.rewrite_old_files)
+	(GLImport.rewrite_new_files): Likewise.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.transitive_closure)
+	(GLModuleTable.transitive_closure_separately): Likewise.
+	* pygnulib/GLTestDir.py (GLTestDir.rewrite_files): Likewise.
+
 2024-04-07  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Don't remove duplicate avoided modules.
diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index cb813b3baa..45462c5fe3 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -879,8 +879,8 @@ AC_DEFUN([%V1%_LIBSOURCES], [
                             capture_output=True)
             if result.returncode == 0:
                 # sort -u
-                emit += lines_to_multiline(sorted(list(set(x.strip()
-                                                           for x in result.stdout.decode(encoding='utf-8').splitlines()))))
+                emit += lines_to_multiline(sorted({ x.strip()
+                                                    for x in result.stdout.decode(encoding='utf-8').splitlines() }))
             else:
                 emit += '== gnulib-tool GNU Make output failed as follows ==\n'
                 emit += ['# stderr: ' + x + '\n' for x in
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index 2776f2c964..08e83c5c9a 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -336,8 +336,7 @@ class GLImport:
             else:  # file is not a special file
                 path = file
             result += [os.path.normpath(path)]
-        result = sorted(set(result))
-        return list(result)
+        return sorted(set(result))
 
     def rewrite_new_files(self, files: list[str]) -> list[str]:
         '''Replace auxdir, docbase, sourcebase, m4base and testsbase from
@@ -373,8 +372,7 @@ class GLImport:
             else:  # file is not a special file
                 path = file
             result += [os.path.normpath(path)]
-        result = sorted(set(result))
-        return list(result)
+        return sorted(set(result))
 
     def actioncmd(self) -> str:
         '''Return command-line invocation comment.'''
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index f8ff71383a..3997668538 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -935,7 +935,7 @@ class GLModuleTable:
             inc_all_tests = self.inc_all_indirect_tests
         modules = sorted(set(outmodules))
         self.modules = modules
-        return list(modules)
+        return modules
 
     def transitive_closure_separately(self, basemodules: list[GLModule],
                                       finalmodules: list[GLModule]) -> tuple[list[GLModule], list[GLModule]]:
@@ -978,7 +978,7 @@ class GLModuleTable:
         #   + [ m
         #       for m in main_modules
         #       if m.getApplicability() != 'main' ]
-        tests_modules = sorted(list(set(tests_modules)))
+        tests_modules = sorted(set(tests_modules))
         # If testsrelated_modules consists only of modules with applicability 'all',
         # set it to empty (because such modules are only helper modules for other modules).
         have_nontrivial_testsrelated_modules = False
diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py
index 829e26fb3f..8defe52fc6 100644
--- a/pygnulib/GLTestDir.py
+++ b/pygnulib/GLTestDir.py
@@ -165,8 +165,7 @@ class GLTestDir:
             else:  # file is not a special file
                 path = file
             result += [os.path.normpath(path)]
-        result = sorted(set(result))
-        return list(result)
+        return sorted(set(result))
 
     def execute(self) -> None:
         '''Create a scratch package with the given modules.'''
-- 
2.44.0


[-- Attachment #3: 0002-gnulib-tool.py-Remove-a-unused-and-incorrect-functio.patch --]
[-- Type: text/x-patch, Size: 1710 bytes --]

From 64eb59f3b2a2677b7e6d7e1090b0189e4f9ce93d Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 7 Apr 2024 18:55:50 -0700
Subject: [PATCH 2/2] gnulib-tool.py: Remove a unused and incorrect function.

* pygnulib/GLFileSystem.py (GLFileAssistant.removeFile): Remove this
unused function. The correct method of removing an element from a list
is to use the remove() function, not pop() which takes an index.
---
 ChangeLog                | 7 +++++++
 pygnulib/GLFileSystem.py | 5 -----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4fd962ecbb..cb563b3830 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-07  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Remove a unused and incorrect function.
+	* pygnulib/GLFileSystem.py (GLFileAssistant.removeFile): Remove this
+	unused function. The correct method of removing an element from a list
+	is to use the remove() function, not pop() which takes an index.
+
 2024-04-07  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Omit some unnecessary list() calls around sorted().
diff --git a/pygnulib/GLFileSystem.py b/pygnulib/GLFileSystem.py
index 551ae177e4..a463575f73 100644
--- a/pygnulib/GLFileSystem.py
+++ b/pygnulib/GLFileSystem.py
@@ -228,11 +228,6 @@ class GLFileAssistant:
         if file not in self.added:
             self.added += [file]
 
-    def removeFile(self, file: str) -> None:
-        '''Remove file from the list of added files.'''
-        if file in self.added:
-            self.added.pop(file)
-
     def getFiles(self) -> list[str]:
         '''Return list of the added files.'''
         return list(self.added)
-- 
2.44.0

Re: gnulib-tool.py: Omit some unnecessary list() calls around sorted().

[Collin Funk]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

Hi Bruno,

I was having a look at GLModuleSystem.py and noticed some duplicate
checks that an key is valid. Patch 0003 addresses that.

Patch 0004 adds a missing None return type hint to
GLModuleTable.getCondition(). This is used to indicate that the module
is not a conditional dependency. I missed this in the type hint patch.

This was also one of the functions you mentioned here:

     https://lists.gnu.org/archive/html/bug-gnulib/2024-03/msg00360.html

> * In GLModuleSystem.py lines 784, 821:
>   A condition can be a string or True. But
>      str | bool
>    makes it look like False was also a valid value. Is possible to
>    write
>      str | True
>    in some way?

The correct way to do this is Literal[True] after the import:

    from typing import Literal

but I wasn't sure if it was compatible with Python 3.7. I ended up
trying it and it is compatible with Python 3.7. The documentation says
Python 3.8 which I've confirmed with my installation [1].

Here is the thread we chose Python 3.7:

    https://lists.gnu.org/archive/html/bug-gnulib/2024-03/msg00004.html

It appears it is EOL but I'm not sure how RHEL / CentOS versions and
packages work [2]. Does the next version default to 3.7? It looks like
they ship a few and allow you to use 'alternatives --set', but I am
unsure which is default.

[1] https://docs.python.org/3/library/typing.html#typing.Literal
[2] https://devguide.python.org/versions

Collin

[-- Attachment #2: 0003-gnulib-tool.py-Remove-unnecessary-conditional.patch --]
[-- Type: text/x-patch, Size: 2147 bytes --]

From 415efd4e122ec5377d890f3e44a992eb7b06986d Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 7 Apr 2024 22:00:48 -0700
Subject: [PATCH 3/4] gnulib-tool.py: Remove unnecessary conditional.

* pygnulib/GLModuleSystem.py (GLModuleTable.__getitem__): Don't check if
the key is valid twice.
---
 ChangeLog                  |  6 ++++++
 pygnulib/GLModuleSystem.py | 21 ++++++++++-----------
 2 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cb563b3830..2bae9994dd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-04-07  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Remove unnecessary conditional.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.__getitem__): Don't check if
+	the key is valid twice.
+
 2024-04-07  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Remove a unused and incorrect function.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 3997668538..d3f38c2b66 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -763,17 +763,16 @@ class GLModuleTable:
 
     def __getitem__(self, y: str) -> list[GLModule]:
         '''x.__getitem__(y) <==> x[y]'''
-        if y in ['base', 'final', 'main', 'tests', 'avoids']:
-            if y == 'base':
-                return self.getBaseModules()
-            elif y == 'final':
-                return self.getFinalModules()
-            elif y == 'main':
-                return self.getMainModules()
-            elif y == 'tests':
-                return self.getTestsModules()
-            else:  # if y == 'avoids'
-                return self.getAvoids()
+        if y == 'base':
+            return self.getBaseModules()
+        elif y == 'final':
+            return self.getFinalModules()
+        elif y == 'main':
+            return self.getMainModules()
+        elif y == 'tests':
+            return self.getTestsModules()
+        elif y == 'avoids':
+            return self.getAvoids()
         else:  # if y is not in list
             raise KeyError('GLModuleTable does not contain key: %s' % repr(y))
 
-- 
2.44.0


[-- Attachment #3: 0004-gnulib-tool.py-Fix-incomplete-type-hint.patch --]
[-- Type: text/x-patch, Size: 1670 bytes --]

From 61f1bf56f271581555e60aa3d20edbc448565eb9 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 7 Apr 2024 22:22:49 -0700
Subject: [PATCH 4/4] gnulib-tool.py: Fix incomplete type hint.

* pygnulib/GLModuleSystem.py (GLModuleTable.getCondition): Add None to
the return type hint. This is the return value when the module is not a
conditional dependency.
---
 ChangeLog                  | 7 +++++++
 pygnulib/GLModuleSystem.py | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 2bae9994dd..0f0aedd2d2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-07  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Fix incomplete type hint.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.getCondition): Add None to
+	the return type hint. This is the return value when the module is not a
+	conditional dependency.
+
 2024-04-07  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Remove unnecessary conditional.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index d3f38c2b66..761c43c2e0 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -813,7 +813,7 @@ class GLModuleTable:
         result = str(module) in self.dependers
         return result
 
-    def getCondition(self, parent: GLModule, module: GLModule) -> str | bool:
+    def getCondition(self, parent: GLModule, module: GLModule) -> str | bool | None:
         '''Return condition from parent to module. Condition can be string or True.
         If module is not in the list of conddeps, method returns None.'''
         if type(parent) is not GLModule:
-- 
2.44.0

Re: gnulib-tool.py: Omit some unnecessary list() calls around sorted().

[Collin Funk]

On 4/7/24 10:57 PM, Collin Funk wrote:
> it is compatible with Python 3.7

It is *not* compatible with Python 3.7.
It only works with Python 3.8+.

My bad...

Collin

gnulib-tool.py: Simplify file list construction.

[Collin Funk]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

Some more cleanup.

Patch 0005 changes a list to a set. Previously, it checked if each
file was a member of the list before appending it. We can just let
Python do this for us. I've also sorted before returning to make it
behave like gnulib-tool.sh. In func_modules_to_filelist:

    files=`for f in $files; do echo $f; done | LC_ALL=C sort -u`

I seem to have not noticed that since now. Hopefully that should help
me cleanup random sorted() calls in other places without breaking the
test suite...

Patch 0006 turns the avoided modules in GLModuleTable into a set
instead of a list. Since we check every module for membership in the
avoided modules, it makes more sense to use a set. The avoided modules
emitted in the actioncmd and gnulib-comp.m4 are stored in GLConfig, so
doing this doesn't break anything.

Patch 0007 uses defaultdict() instead of dict() for module
dependencies. This has been around for a long time and deals with the
explicit initialization case for you:

>>> var = dict()
>>> var['a'].add(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
KeyError: 'a'
>>> from collections import defaultdict
>>> var = defaultdict(set)
>>> var['a'].add(1)
>>> print(var)
defaultdict(<class 'set'>, {'a': {1}})

[1] https://docs.python.org/3/library/collections.html#defaultdict-objects

Collin

[-- Attachment #2: 0005-gnulib-tool.py-Simplify-file-list-construction.patch --]
[-- Type: text/x-patch, Size: 2172 bytes --]

From 24af1bb4778f683be73726a3e0b47a022dd75196 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Mon, 8 Apr 2024 01:48:41 -0700
Subject: [PATCH 5/7] gnulib-tool.py: Simplify file list construction.

* pygnulib/GLModuleSystem.py (GLModuleTable.filelist): Use a set to
construct the file list instead of looping through a list for each file.
Sort the result to match gnulib-tool.sh.
---
 ChangeLog                  | 7 +++++++
 pygnulib/GLModuleSystem.py | 7 +++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 0f0aedd2d2..aaa4d63652 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-08  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Simplify file list construction.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.filelist): Use a set to
+	construct the file list instead of looping through a list for each file.
+	Sort the result to match gnulib-tool.sh.
+
 2024-04-07  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Fix incomplete type hint.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 761c43c2e0..1532610466 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -1047,7 +1047,7 @@ class GLModuleTable:
     def filelist(self, modules: list[GLModule]) -> list[str]:
         '''Determine the final file list for the given list of modules.
         The list of modules must already include dependencies.'''
-        filelist = []
+        fileset = set()
         for module in modules:
             if type(module) is not GLModule:
                 raise TypeError('each module must be a GLModule instance')
@@ -1055,9 +1055,8 @@ class GLModuleTable:
                      for module in modules ]
         for listing in listings:
             for file in listing:
-                if file not in filelist:
-                    filelist += [file]
-        return filelist
+                fileset.add(file)
+        return sorted(fileset)
 
     def filelist_separately(self, main_modules: list[GLModule],
                             tests_modules: list[GLModule]) -> tuple[list[str], list[str]]:
-- 
2.44.0


[-- Attachment #3: 0006-gnulib-tool.py-Use-a-set-instead-of-list-for-avoided.patch --]
[-- Type: text/x-patch, Size: 2490 bytes --]

From e9154caf868123ab2558495e831ffd9e097b6c07 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Mon, 8 Apr 2024 02:58:49 -0700
Subject: [PATCH 6/7] gnulib-tool.py: Use a set instead of list for avoided
 module checks.

* pygnulib/GLModuleSystem.py (GLModuleTable.__init__): Use a set instead
of a list for avoided modules. This is used only for membership checks
when computing the transitive closure of the given modules, therefore
prefer the O(1) average case over O(n).
(GLModuleTable.setAvoids): Remove sorted() call.
---
 ChangeLog                  | 9 +++++++++
 pygnulib/GLModuleSystem.py | 6 +++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index aaa4d63652..db7b7ca3ec 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2024-04-08  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Use a set instead of list for avoided module checks.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.__init__): Use a set instead
+	of a list for avoided modules. This is used only for membership checks
+	when computing the transitive closure of the given modules, therefore
+	prefer the O(1) average case over O(n).
+	(GLModuleTable.setAvoids): Remove sorted() call.
+
 2024-04-08  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Simplify file list construction.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 1532610466..60e9846a66 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -750,11 +750,11 @@ class GLModuleTable:
                             % type(inc_all_direct_tests).__name__)
         self.inc_all_direct_tests = inc_all_direct_tests
         self.inc_all_indirect_tests = inc_all_indirect_tests
-        self.avoids = []  # Avoids
+        self.avoids = set()  # Avoids
         for avoid in self.config.getAvoids():
             module = self.modulesystem.find(avoid)
             if module:
-                self.avoids.append(module)
+                self.avoids.add(module)
 
     def __repr__(self) -> str:
         '''x.__repr__() <==> repr(x)'''
@@ -1080,7 +1080,7 @@ class GLModuleTable:
         for module in modules:
             if type(module) is not GLModule:
                 raise TypeError('each module must be a GLModule instance')
-        self.avoids = sorted(set(modules))
+        self.avoids = set(modules)
 
     def getBaseModules(self) -> list[GLModule]:
         '''Return list of base modules.'''
-- 
2.44.0


[-- Attachment #4: 0007-gnulib-tool.py-Use-a-defaultdict-to-simplify-code.patch --]
[-- Type: text/x-patch, Size: 2588 bytes --]

From 60cf5da7155a4b4eb255704e76124981928a300a Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Mon, 8 Apr 2024 03:41:53 -0700
Subject: [PATCH 7/7] gnulib-tool.py: Use a defaultdict to simplify code.

* pygnulib/GLModuleSystem.py (GLModuleTable.__init__): Use a defaultdict
so the initial value for a key is handled for us.
(GLModuleTable.addConditional): Remove the initial value case.
---
 ChangeLog                  | 7 +++++++
 pygnulib/GLModuleSystem.py | 8 +++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index db7b7ca3ec..e4b70568ab 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2024-04-08  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Use a defaultdict to simplify code.
+	* pygnulib/GLModuleSystem.py (GLModuleTable.__init__): Use a defaultdict
+	so the initial value for a key is handled for us.
+	(GLModuleTable.addConditional): Remove the initial value case.
+
 2024-04-08  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Use a set instead of list for avoided module checks.
diff --git a/pygnulib/GLModuleSystem.py b/pygnulib/GLModuleSystem.py
index 60e9846a66..a190b053ab 100644
--- a/pygnulib/GLModuleSystem.py
+++ b/pygnulib/GLModuleSystem.py
@@ -24,6 +24,7 @@ import sys
 import codecs
 import hashlib
 import subprocess as sp
+from collections import defaultdict
 from . import constants
 from .GLError import GLError
 from .GLConfig import GLConfig
@@ -732,7 +733,7 @@ class GLModuleTable:
           returns the condition when B should be enabled as a dependency of A,
           once the m4 code for A has been executed.
         '''
-        self.dependers = dict()  # Dependencies
+        self.dependers = defaultdict(list)  # Dependencies
         self.conditionals = dict()  # Conditional modules
         self.unconditionals = dict()  # Unconditional modules
         self.base_modules = []  # Base modules
@@ -789,10 +790,7 @@ class GLModuleTable:
                             % type(condition).__name__)
         if not str(module) in self.unconditionals:
             # No unconditional dependency to the given module is known at this point.
-            if str(module) not in self.dependers:
-                self.dependers[str(module)] = []
-            if str(parent) not in self.dependers[str(module)]:
-                self.dependers[str(module)].append(str(parent))
+            self.dependers[str(module)].append(str(parent))
             key = '%s---%s' % (str(parent), str(module))
             self.conditionals[key] = condition
 
-- 
2.44.0

Re: gnulib-tool.py: Omit some unnecessary list() calls around sorted().

[Bruno Haible]

Hi Collin,

> Here is two janitorial work patches.

Thanks, applied.

> I remember an extra dict() call causing trouble with GLMakefileTable.
> Removing the extra list() calls from sorted() seemed like a good place
> to start cleaning up.

Well, that extra dict() call made trouble because in that particular case,
a side effect was desired. Usually, side effects are not desired and,
if they occur, lead to bugs that are long to debug. That was the motivation
for Dmitry's many list() and dict() calls. But now that we have a test suite,
we can drop this extra precaution.

The general rule (in most software projects) is: Do not make side effects on
the values returned by other functions. Make side effects only
  - on local variables,
  - on returned values, if that is intentional.

Things are different for example in libraries that are meant to be used by
other software developers. Since you cannot assume that these developers will
always follow this rule without making mistakes, the common approach there
is that functions return a copy of the internal data, so that side effects
will not have effects that look like library bugs.

Bruno

Re: gnulib-tool.py: Omit some unnecessary list() calls around sorted().

[Bruno Haible]

Hi Collin,

> I was having a look at GLModuleSystem.py and noticed some duplicate
> checks that an key is valid. Patch 0003 addresses that.
> 
> Patch 0004 adds a missing None return type hint to
> GLModuleTable.getCondition(). This is used to indicate that the module
> is not a conditional dependency. I missed this in the type hint patch.

Looks good. Both patches applied.

> > * In GLModuleSystem.py lines 784, 821:
> >   A condition can be a string or True. But
> >      str | bool
> >    makes it look like False was also a valid value. Is possible to
> >    write
> >      str | True
> >    in some way?
> 
> The correct way to do this is Literal[True] after the import:
> 
>     from typing import Literal
> 
> but I wasn't sure if it was compatible with Python 3.7. I ended up
> trying it and it is compatible with Python 3.7. The documentation says
> Python 3.8 which I've confirmed with my installation [1].

OK; if it's not valid syntax in Python 3.7 then we can't use it (or
use it only in comments).

> Here is the thread we chose Python 3.7:
> 
>     https://lists.gnu.org/archive/html/bug-gnulib/2024-03/msg00004.html
> 
> It appears it is EOL but I'm not sure how RHEL / CentOS versions and
> packages work [2]. Does the next version default to 3.7? It looks like
> they ship a few and allow you to use 'alternatives --set', but I am
> unsure which is default.

I'm not willing to open this discussion of minimum Python version again
now (barring new arguments). We have considered everything, then made
a decision; now let's stick to that decision. We can revisit this in
two years.

Bruno

Re: gnulib-tool.py: Simplify file list construction.

[Bruno Haible]

Hi Collin,

> Patch 0005 changes a list to a set. Previously, it checked if each
> file was a member of the list before appending it. We can just let
> Python do this for us. I've also sorted before returning to make it
> behave like gnulib-tool.sh.

Typically, when one changes a function, one should ask oneself
"should the doc string be updated?"

So. Should the doc string include a hint
   '''The resulting list is sorted.''' ?
No, that would be an over-specification of this function's behaviour.
The callers of this functions will sort themselves.

So. Should the doc string say
   '''The sorting order order on the resulting list is unspecified.
   The caller needs to sort according to their criteria.'''

This is better. But wait: If a return value is a list with unspecified
sorting order, and doesn't contain duplicates, then list is the wrong
data type. The function should return a set then.

=> Not applied.

> Patch 0006 turns the avoided modules in GLModuleTable into a set
> instead of a list. Since we check every module for membership in the
> avoided modules, it makes more sense to use a set. The avoided modules
> emitted in the actioncmd and gnulib-comp.m4 are stored in GLConfig, so
> doing this doesn't break anything.

This patch goes into a right direction. But when the field contains a set,
the accessors getAvoids and setAvoids should IMO also return a set and
accept a set, respectively.

=> Not applied.

> Patch 0007 uses defaultdict() instead of dict() for module
> dependencies. This has been around for a long time and deals with the
> explicit initialization case for you:

It's a nice Python class. But the use of defaultdict(list) makes only
the two lines

            if str(module) not in self.dependers:
                self.dependers[str(module)] = []

redundant. It does not make the line

            if str(parent) not in self.dependers[str(module)]:

redundant. self.dependers[str(module)] should not contain duplicates.

=> Not applied.

Bruno

Re: gnulib-tool.py: Simplify file list construction.

[Collin Funk]

Hi Bruno,

On 4/8/24 12:30 PM, Bruno Haible wrote:
> Typically, when one changes a function, one should ask oneself
> "should the doc string be updated?"

Yes, I forget this sometimes...

> This is better. But wait: If a return value is a list with unspecified
> sorting order, and doesn't contain duplicates, then list is the wrong
> data type. The function should return a set then.

Makes sense.

> This patch goes into a right direction. But when the field contains a set,
> the accessors getAvoids and setAvoids should IMO also return a set and
> accept a set, respectively.

I don't think the GLModuleTable.getAvoids() is used. But it is
difficult to tell because we also have GLConfig.getAvoids(). And for
both of those we have these to access them:

     var = table['avoids']   # table is a GLModuleTable
     var = config['avoids']  # config is a GLConfig

So it is difficult to tell. I know we have this in GLConfig for the
cache handling, but I don't think it is important for GLModuleTable.
I'll look into that more.

> It does not make the line
> 
>             if str(parent) not in self.dependers[str(module)]:
> 
> redundant. self.dependers[str(module)] should not contain duplicates.

Ah, okay. Which makes me question whether sorting is important. I do
not believe so, but I have to double check. If we don't care about
sorting then a set would work in that case.

Collin