bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
* gnulib-tool.py: Remove a redundant function.
@ 2024-04-15  1:23 Collin Funk
  2024-04-15  2:20 ` Collin Funk
  2024-04-15 11:47 ` gnulib-tool.py: Remove a redundant function Bruno Haible
  0 siblings, 2 replies; 12+ messages in thread
From: Collin Funk @ 2024-04-15  1:23 UTC (permalink / raw)
  To: bug-gnulib

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

The GLImport class has two functions that are the same,
GLImport.rewrite_old_files() and GLImport.rewrite_new_files().

The GLImport.rewrite_old_files() function does this extra step before
processing the list:

        files = [ '%s%s' % (file, os.path.sep)
                   for file in files ]

But before appending it to the resulting list os.path.normpath() is
used.

Since the following is true:

      os.path.normpath('abc') == 'abc'
      os.path.normpath('abc/') == 'abc'

both of these functions are the same.

Therefore, we can remove GLImport.rewrite_old_files() and rename
GLImport.rewrite_new_files() to GLImport.rewrite_files().

Also, I noticed we have:

         for src in old_files:
             dest = self.rewrite_files([src])[-1]
             old_table.append(tuple([dest, src]))

This is looping over a list, creating a new list with one item,
calling GLImport.rewrite_files(), which then calls sorted(set(...))
twice, and then appending the result to a list.

We should be able to create a new list from that function and zip()
the two together. I'll submit another patch for that since it requires
some sorting changes.

Collin

[-- Attachment #2: 0001-gnulib-tool.py-Remove-a-redundant-function.patch --]
[-- Type: text/x-patch, Size: 4193 bytes --]

From ec3d2f70a06e93b6cd730dd1f3629d5a6ad386fc Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 14 Apr 2024 18:09:39 -0700
Subject: [PATCH] gnulib-tool.py: Remove a redundant function.

* pygnulib/GLImport.py (GLImport.rewrite_old_files): Remove function.
(GLImport.rewrite_new_files): Rename to rewrite_files.
(GLImport.prepare): Use rewrite_files instead of rewrite_old_files and
rewrite_new_files.
---
 ChangeLog            |  8 ++++++++
 pygnulib/GLImport.py | 44 +++-----------------------------------------
 2 files changed, 11 insertions(+), 41 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 5b7b3a36fc..82c4a5f838 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-04-14  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Remove a redundant function.
+	* pygnulib/GLImport.py (GLImport.rewrite_old_files): Remove function.
+	(GLImport.rewrite_new_files): Rename to rewrite_files.
+	(GLImport.prepare): Use rewrite_files instead of rewrite_old_files and
+	rewrite_new_files.
+
 2024-04-14  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Fix incorrect type hint.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index c6a4693c90..430691efbd 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -314,45 +314,7 @@ def __repr__(self) -> str:
         result = '<pygnulib.GLImport %s>' % hex(id(self))
         return result
 
-    def rewrite_old_files(self, files: list[str]) -> list[str]:
-        '''Replace auxdir, docbase, sourcebase, m4base and testsbase from default
-        to their version from cached config.'''
-        if type(files) is not list:
-            raise TypeError('files argument must has list type, not %s'
-                            % type(files).__name__)
-        for file in files:
-            if type(file) is not str:
-                raise TypeError('each file must be a string instance')
-        files = sorted(set(files))
-        files = [ '%s%s' % (file, os.path.sep)
-                  for file in files ]
-        auxdir = self.cache['auxdir']
-        docbase = self.cache['docbase']
-        sourcebase = self.cache['sourcebase']
-        m4base = self.cache['m4base']
-        testsbase = self.cache['testsbase']
-        result = []
-        for file in files:
-            if file.startswith('build-aux/'):
-                path = constants.substart('build-aux/', '%s/' % auxdir, file)
-            elif file.startswith('doc/'):
-                path = constants.substart('doc/', '%s/' % docbase, file)
-            elif file.startswith('lib/'):
-                path = constants.substart('lib/', '%s/' % sourcebase, file)
-            elif file.startswith('m4/'):
-                path = constants.substart('m4/', '%s/' % m4base, file)
-            elif file.startswith('tests/'):
-                path = constants.substart('tests/', '%s/' % testsbase, file)
-            elif file.startswith('tests=lib/'):
-                path = constants.substart('tests=lib/', '%s/' % testsbase, file)
-            elif file.startswith('top/'):
-                path = constants.substart('top/', '', file)
-            else:  # file is not a special file
-                path = file
-            result.append(os.path.normpath(path))
-        return sorted(set(result))
-
-    def rewrite_new_files(self, files: list[str]) -> list[str]:
+    def rewrite_files(self, files: list[str]) -> list[str]:
         '''Replace auxdir, docbase, sourcebase, m4base and testsbase from
         default to their version from config.'''
         if type(files) is not list:
@@ -959,10 +921,10 @@ def prepare(self) -> tuple[dict[str, list[str]], dict[str, str]]:
         old_table = []
         new_table = []
         for src in old_files:
-            dest = self.rewrite_old_files([src])[-1]
+            dest = self.rewrite_files([src])[-1]
             old_table.append(tuple([dest, src]))
         for src in new_files:
-            dest = self.rewrite_new_files([src])[-1]
+            dest = self.rewrite_files([src])[-1]
             new_table.append(tuple([dest, src]))
         old_table = sorted(set(old_table))
         new_table = sorted(set(new_table))
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-15  1:23 gnulib-tool.py: Remove a redundant function Collin Funk
@ 2024-04-15  2:20 ` Collin Funk
  2024-04-15 14:58   ` Bruno Haible
  2024-04-15 11:47 ` gnulib-tool.py: Remove a redundant function Bruno Haible
  1 sibling, 1 reply; 12+ messages in thread
From: Collin Funk @ 2024-04-15  2:20 UTC (permalink / raw)
  To: bug-gnulib

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

On 4/14/24 6:23 PM, Collin Funk wrote:
> Also, I noticed we have:
> 
>          for src in old_files:
>              dest = self.rewrite_files([src])[-1]
>              old_table.append(tuple([dest, src]))
> 
> This is looping over a list, creating a new list with one item,
> calling GLImport.rewrite_files(), which then calls sorted(set(...))
> twice, and then appending the result to a list.
> 
> We should be able to create a new list from that function and zip()
> the two together. I'll submit another patch for that since it requires
> some sorting changes.

Patch 0002 does this. GLTestDir also has this rewrite_files() function
so I did the same there. Maybe it is worth making that a helper
function or using a base class in the future.

Also, the set() and list() calls around zip(...) are important since
zip() returns an iterator [1]. I've used whichever was most similar to
the previous code.

Patch 0003 removes a directories list that was unused. These are
created in the loop below it as files are written.

[1] https://docs.python.org/3/library/functions.html#zip

Collin

[-- Attachment #2: 0002-gnulib-tool.py-Refactor-file-name-transformations.patch --]
[-- Type: text/x-patch, Size: 4633 bytes --]

From 3ca340c6a56d25e3e87786d12c04fcab2f8d0973 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 14 Apr 2024 18:55:36 -0700
Subject: [PATCH 2/3] gnulib-tool.py: Refactor file name transformations.

* pygnulib/GLImport.py (GLImport.rewrite_files): Don't sort and don't
remove duplicates.
(GLImport.prepare): Pass the the file list to rewrite_files and zip
it together the result.
* pygnulib/GLTestDir.py (GLTestDir.rewrite_files): Don't sort and don't
remove duplicates.
(GLTestDir.execute): Pass the the file list to rewrite_files and zip
it together the result.
---
 ChangeLog             | 12 ++++++++++++
 pygnulib/GLImport.py  | 15 +++------------
 pygnulib/GLTestDir.py |  8 ++------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 82c4a5f838..cf52f56b19 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2024-04-14  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Refactor file name transformations.
+	* pygnulib/GLImport.py (GLImport.rewrite_files): Don't sort and don't
+	remove duplicates.
+	(GLImport.prepare): Pass the the file list to rewrite_files and zip
+	it together the result.
+	* pygnulib/GLTestDir.py (GLTestDir.rewrite_files): Don't sort and don't
+	remove duplicates.
+	(GLTestDir.execute): Pass the the file list to rewrite_files and zip
+	it together the result.
+
 2024-04-14  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Remove a redundant function.
diff --git a/pygnulib/GLImport.py b/pygnulib/GLImport.py
index 430691efbd..ceacfbb232 100644
--- a/pygnulib/GLImport.py
+++ b/pygnulib/GLImport.py
@@ -323,7 +323,6 @@ def rewrite_files(self, files: list[str]) -> list[str]:
         for file in files:
             if type(file) is not str:
                 raise TypeError('each file must be a string instance')
-        files = sorted(set(files))
         auxdir = self.config['auxdir']
         docbase = self.config['docbase']
         sourcebase = self.config['sourcebase']
@@ -348,7 +347,7 @@ def rewrite_files(self, files: list[str]) -> list[str]:
             else:  # file is not a special file
                 path = file
             result.append(os.path.normpath(path))
-        return sorted(set(result))
+        return result
 
     def actioncmd(self) -> str:
         '''Return command-line invocation comment.'''
@@ -918,16 +917,8 @@ def prepare(self) -> tuple[dict[str, list[str]], dict[str, str]]:
         transformers['aux'] = sed_transform_build_aux_file
         transformers['main'] = sed_transform_main_lib_file
         transformers['tests'] = sed_transform_testsrelated_lib_file
-        old_table = []
-        new_table = []
-        for src in old_files:
-            dest = self.rewrite_files([src])[-1]
-            old_table.append(tuple([dest, src]))
-        for src in new_files:
-            dest = self.rewrite_files([src])[-1]
-            new_table.append(tuple([dest, src]))
-        old_table = sorted(set(old_table))
-        new_table = sorted(set(new_table))
+        old_table = sorted(set(zip(self.rewrite_files(old_files), old_files)))
+        new_table = sorted(set(zip(self.rewrite_files(new_files), new_files)))
 
         # Prepare the filetable.
         filetable = dict()
diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py
index a7709a1259..dee8d629dc 100644
--- a/pygnulib/GLTestDir.py
+++ b/pygnulib/GLTestDir.py
@@ -138,7 +138,6 @@ def rewrite_files(self, files: list[str]) -> list[str]:
         for file in files:
             if type(file) is not str:
                 raise TypeError('each file must be a string instance')
-        files = sorted(set(files))
         auxdir = self.config['auxdir']
         docbase = self.config['docbase']
         sourcebase = self.config['sourcebase']
@@ -163,7 +162,7 @@ def rewrite_files(self, files: list[str]) -> list[str]:
             else:  # file is not a special file
                 path = file
             result.append(os.path.normpath(path))
-        return sorted(set(result))
+        return result
 
     def execute(self) -> None:
         '''Create a scratch package with the given modules.'''
@@ -350,10 +349,7 @@ def execute(self) -> None:
         directories = sorted(set(directories))
 
         # Copy files or make symbolic links or hard links.
-        filetable = []
-        for src in filelist:
-            dest = self.rewrite_files([src])[-1]
-            filetable.append(tuple([dest, src]))
+        filetable = list(zip(self.rewrite_files(filelist), filelist))
         for row in filetable:
             src = row[1]
             dest = row[0]
-- 
2.44.0


[-- Attachment #3: 0003-gnulib-tool.py-Remove-an-unused-variable.patch --]
[-- Type: text/x-patch, Size: 1521 bytes --]

From fef3a610332a955c6ca29fdecdcb719fde4f7162 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.funk1@gmail.com>
Date: Sun, 14 Apr 2024 19:00:35 -0700
Subject: [PATCH 3/3] gnulib-tool.py: Remove an unused variable.

* pygnulib/GLTestDir.py (GLTestDir.execute): Remove the unused
directories variable.
---
 ChangeLog             | 6 ++++++
 pygnulib/GLTestDir.py | 5 -----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index cf52f56b19..32e454d32e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2024-04-14  Collin Funk  <collin.funk1@gmail.com>
+
+	gnulib-tool.py: Remove an unused variable.
+	* pygnulib/GLTestDir.py (GLTestDir.execute): Remove the unused
+	directories variable.
+
 2024-04-14  Collin Funk  <collin.funk1@gmail.com>
 
 	gnulib-tool.py: Refactor file name transformations.
diff --git a/pygnulib/GLTestDir.py b/pygnulib/GLTestDir.py
index dee8d629dc..8ebe28b70a 100644
--- a/pygnulib/GLTestDir.py
+++ b/pygnulib/GLTestDir.py
@@ -343,11 +343,6 @@ def execute(self) -> None:
         filelist += ['build-aux/config.guess', 'build-aux/config.sub']
         filelist = sorted(set(filelist))
 
-        # Create directories.
-        directories = [os.path.dirname(file)
-                       for file in self.rewrite_files(filelist)]
-        directories = sorted(set(directories))
-
         # Copy files or make symbolic links or hard links.
         filetable = list(zip(self.rewrite_files(filelist), filelist))
         for row in filetable:
-- 
2.44.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-15  1:23 gnulib-tool.py: Remove a redundant function Collin Funk
  2024-04-15  2:20 ` Collin Funk
@ 2024-04-15 11:47 ` Bruno Haible
  2024-04-15 14:07   ` Collin Funk
  1 sibling, 1 reply; 12+ messages in thread
From: Bruno Haible @ 2024-04-15 11:47 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Collin Funk

Collin Funk wrote:
> The GLImport class has two functions that are the same,
> GLImport.rewrite_old_files() and GLImport.rewrite_new_files().

No. When I copy these functions into separate text files and use 'diff'
on them:

$ diff -u 1 2
--- 1   2024-04-15 06:34:45.441369330 +0200
+++ 2   2024-04-15 06:35:01.941511954 +0200
@@ -1,7 +1,7 @@
 
-    def rewrite_old_files(self, files: list[str]) -> list[str]:
-        '''Replace auxdir, docbase, sourcebase, m4base and testsbase from default
-        to their version from cached config.'''
+    def rewrite_new_files(self, files: list[str]) -> list[str]:
+        '''Replace auxdir, docbase, sourcebase, m4base and testsbase from
+        default to their version from config.'''
         if type(files) is not list:
             raise TypeError('files argument must has list type, not %s'
                             % type(files).__name__)
@@ -9,13 +9,11 @@
             if type(file) is not str:
                 raise TypeError('each file must be a string instance')
         files = sorted(set(files))
-        files = [ '%s%s' % (file, os.path.sep)
-                  for file in files ]
-        auxdir = self.cache['auxdir']
-        docbase = self.cache['docbase']
-        sourcebase = self.cache['sourcebase']
-        m4base = self.cache['m4base']
-        testsbase = self.cache['testsbase']
+        auxdir = self.config['auxdir']
+        docbase = self.config['docbase']
+        sourcebase = self.config['sourcebase']
+        m4base = self.config['m4base']
+        testsbase = self.config['testsbase']
         result = []
         for file in files:
             if file.startswith('build-aux/'):

> Therefore, we can remove GLImport.rewrite_old_files() and rename
> GLImport.rewrite_new_files() to GLImport.rewrite_files().

No. I'm adding 3 unit tests that prove that the patch is wrong,
one for each of docbase, sourcebase, testsbase. (For auxdir and m4base
gnulib-tool.{sh,py} does not support changing the value while preserving
the rest: For auxdir the old files are not removed, for m4base the list
of modules gets reset to empty.)

Bruno





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-15 11:47 ` gnulib-tool.py: Remove a redundant function Bruno Haible
@ 2024-04-15 14:07   ` Collin Funk
  0 siblings, 0 replies; 12+ messages in thread
From: Collin Funk @ 2024-04-15 14:07 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib

Hi Bruno,

On 4/15/24 4:47 AM, Bruno Haible wrote:
> No. I'm adding 3 unit tests that prove that the patch is wrong,
> one for each of docbase, sourcebase, testsbase. (For auxdir and m4base
> gnulib-tool.{sh,py} does not support changing the value while preserving
> the rest: For auxdir the old files are not removed, for m4base the list
> of modules gets reset to empty.)

Ah, I see. I should have diff'd the two functions like you. The
'self.config' and 'self.cache' was too easy for my eyes to overlook...
Thanks for adding the tests. My local branch fails but passes using
master, so I can see this change is incorrect.

I sent 2 other patches whenever you have time to check them. I can
rewrite patch 0002 as long as it is otherwise correct. It should use
rewrite_{new,old}_files and not the improper renamed function from
patch 0001.

Patch 0003 should work fine. Just an unused variable that is handled
elsewhere.

Collin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-15  2:20 ` Collin Funk
@ 2024-04-15 14:58   ` Bruno Haible
  2024-04-15 15:24     ` Collin Funk
  0 siblings, 1 reply; 12+ messages in thread
From: Bruno Haible @ 2024-04-15 14:58 UTC (permalink / raw)
  To: bug-gnulib; +Cc: Collin Funk

Hi Collin,

> Patch 0002 does this. GLTestDir also has this rewrite_files() function
> so I did the same there. Maybe it is worth making that a helper
> function or using a base class in the future.
> 
> Also, the set() and list() calls around zip(...) are important since
> zip() returns an iterator [1]. I've used whichever was most similar to
> the previous code.

Patch 0002 is not applicable because it relies on 0001, which was not good.

Also, the last hunk makes use of yet another Python built-in function 'zip',
where list comprehension [ ... for ... in ... ] is more readable.

> Patch 0003 removes a directories list that was unused. These are
> created in the loop below it as files are written.

In gnulib-tool.sh the directories are created ahead of the loop that
copies the files. Why? Because when we have to create 500 files in the
lib/ directory, it is faster to do 'if not isdir(dirname)' once than
500 times. This is also true in Python.

Bruno





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-15 14:58   ` Bruno Haible
@ 2024-04-15 15:24     ` Collin Funk
  2024-04-16 15:09       ` Bruno Haible
  0 siblings, 1 reply; 12+ messages in thread
From: Collin Funk @ 2024-04-15 15:24 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib

Hi Bruno,

On 4/15/24 7:58 AM, Bruno Haible wrote:
> Patch 0002 is not applicable because it relies on 0001, which was not good.

Yes, I need to rewrite it. But I think the idea of the patch is still
correct. Since it doesn't make sense to accept a list and then only
use it with one element lists.

> Also, the last hunk makes use of yet another Python built-in function 'zip',
> where list comprehension [ ... for ... in ... ] is more readable.

Maybe I am missing something, but I don't think there is a good way to
use a list comprehension here without 'zip'. Since 'zip' is used to
combine these two lists like so:

        list1 = [ 1, 2, 3, 4 ]
        list2 = [ 5, 6, 7, 8 ]
        result = list(zip(list1, list2))
        print(result)
        [(1, 5), (2, 6), (3, 7), (4, 8)]

This still uses 'zip' but maybe you find it easier to read?

     result = [ (a, b) for
                a, b in zip(list1, list2) ]

> In gnulib-tool.sh the directories are created ahead of the loop that
> copies the files. Why? Because when we have to create 500 files in the
> lib/ directory, it is faster to do 'if not isdir(dirname)' once than
> 500 times. This is also true in Python.

Ah, yes that makes sense. I'll go fix that now.

Collin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-15 15:24     ` Collin Funk
@ 2024-04-16 15:09       ` Bruno Haible
  2024-04-16 16:23         ` Collin Funk
  2024-04-17  1:12         ` Refactoring rewrite_filename functions Collin Funk
  0 siblings, 2 replies; 12+ messages in thread
From: Bruno Haible @ 2024-04-16 15:09 UTC (permalink / raw)
  To: bug-gnulib, Collin Funk

Hi Collin,

> But I think the idea of the patch is still
> correct. Since it doesn't make sense to accept a list and then only
> use it with one element lists.

Sure. This code structure comes from the fact that in the shell
implementation, the rewriting of file names is done through a 'sed' invocation,
and that is equally suited to a single file name or a list of file names.

> > Also, the last hunk makes use of yet another Python built-in function 'zip',
> > where list comprehension [ ... for ... in ... ] is more readable.
> 
> Maybe I am missing something, but I don't think there is a good way to
> use a list comprehension here without 'zip'. Since 'zip' is used to
> combine these two lists like so:

I'm talking about this piece of code:

        filetable = []
        for src in filelist:
            dest = self.rewrite_files([src])[-1]
            filetable.append(tuple([dest, src]))

which can be written as

        filetable = [ tuple([self.rewrite_filename(src), src])
                      for src in filelist ]

Bruno





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: gnulib-tool.py: Remove a redundant function.
  2024-04-16 15:09       ` Bruno Haible
@ 2024-04-16 16:23         ` Collin Funk
  2024-04-17  1:12         ` Refactoring rewrite_filename functions Collin Funk
  1 sibling, 0 replies; 12+ messages in thread
From: Collin Funk @ 2024-04-16 16:23 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib

Hi Bruno,

On 4/16/24 8:09 AM, Bruno Haible wrote:
> I'm talking about this piece of code:
> 
>         filetable = []
>         for src in filelist:
>             dest = self.rewrite_files([src])[-1]
>             filetable.append(tuple([dest, src]))
> 
> which can be written as
> 
>         filetable = [ tuple([self.rewrite_filename(src), src])
>                       for src in filelist ]

Ah, okay now I see what you mean. I'll have a look at doing something
like that. Thanks!

Collin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Refactoring rewrite_filename functions
  2024-04-16 15:09       ` Bruno Haible
  2024-04-16 16:23         ` Collin Funk
@ 2024-04-17  1:12         ` Collin Funk
  2024-04-17  1:35           ` Bruno Haible
  1 sibling, 1 reply; 12+ messages in thread
From: Collin Funk @ 2024-04-17  1:12 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib

Hi Bruno,

On 4/16/24 8:09 AM, Bruno Haible wrote:
> I'm talking about this piece of code:
> 
>         filetable = []
>         for src in filelist:
>             dest = self.rewrite_files([src])[-1]
>             filetable.append(tuple([dest, src]))
> 
> which can be written as
> 
>         filetable = [ tuple([self.rewrite_filename(src), src])
>                       for src in filelist ]

I'm taking a look at this again and I think it would make sense to
move these 'rewrite_filename' functions to GLConfig since they only
use the directory names stored there. Right now we have the following:

    1. GLImport.rewrite_old_files()  # Uses self.cache
    2. GLImport.rewrite_new_files()  # Uses self.config
    3. GLTestDir.rewrite_files()     # Uses self.config, TestDir has no cache

I'm thinking of making this function accept a single filename instead
of a list, and then moving it to GLConfig. Omitting the tuple stuff
for clarity, but this is how the new/old filename distinction would be
handled:

     # Equals current GLImport.rewrite_old_files()
     [ self.cache.rewrite_filename(src)
       for src in filelist ]

     # Equals current GLImport.rewrite_new_files()
     [ self.config.rewrite_filename(src)
       for src in filelist ]

What do you think about this change?

Also, there is a similar section of code to this new function in
main.py line 1313 under "mode == 'copy-file'", but it is missing the
'tests=lib/' replacement. Would there be a way to simplify that too?
The use of 'tests=lib/' is escaping my mind at the moment...

Collin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Refactoring rewrite_filename functions
  2024-04-17  1:12         ` Refactoring rewrite_filename functions Collin Funk
@ 2024-04-17  1:35           ` Bruno Haible
  2024-04-17  2:08             ` Collin Funk
  0 siblings, 1 reply; 12+ messages in thread
From: Bruno Haible @ 2024-04-17  1:35 UTC (permalink / raw)
  To: bug-gnulib, Collin Funk

Hi Collin,

> I'm thinking of making this function accept a single filename instead
> of a list

Makes sense, as previously discussed.

> and then moving it to GLConfig.

This does not make sense. GLConfig is meant to hold settings and configuration,
nothing more.

I see the home of this function more in GLFileSystem.py. Maybe in class
GLFileAssistant, maybe in a new class in that same file.

> Also, there is a similar section of code to this new function in
> main.py line 1313 under "mode == 'copy-file'", but it is missing the
> 'tests=lib/' replacement. Would there be a way to simplify that too?
> The use of 'tests=lib/' is escaping my mind at the moment...

'tests=lib/' should not occur in the scope of mode == 'copy-file'.
But this string is an indicator inside gnulib-tool; users of gnulib-tool
should not be allowed to pass it.

Bruno





^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Refactoring rewrite_filename functions
  2024-04-17  1:35           ` Bruno Haible
@ 2024-04-17  2:08             ` Collin Funk
  2024-04-17 14:03               ` Bruno Haible
  0 siblings, 1 reply; 12+ messages in thread
From: Collin Funk @ 2024-04-17  2:08 UTC (permalink / raw)
  To: Bruno Haible, bug-gnulib

On 4/16/24 6:35 PM, Bruno Haible wrote:
> This does not make sense. GLConfig is meant to hold settings and configuration,
> nothing more.
> 
> I see the home of this function more in GLFileSystem.py. Maybe in class
> GLFileAssistant, maybe in a new class in that same file.

Hmm, thanks for the advice. At the moment I think GLFileAssistant
makes sense. Since it performs transformations on the files based on
their categories, transforming the path names seems somewhat in its
domain.

This will require some code reordering first though. The
GLFileAssistant used by GLImport is not defined until
GLImport.execute(). The file name transformations are done in
GLImport.prepare(). This function also returns the transformers used
by GLFileAssistant.__init__().

I think the correct solution there is to define the GLFileAssistant in
GLImport.__init__() with it's default arguments and then set them
later. I've been meaning to do this anyways because I find defining
things outside of __init__ makes things harder to follow. This was
also a reason I had in the back of my head when fixing the default
arguments here:

    https://lists.gnu.org/archive/html/bug-gnulib/2024-04/msg00228.html

> 'tests=lib/' should not occur in the scope of mode == 'copy-file'.
> But this string is an indicator inside gnulib-tool; users of gnulib-tool
> should not be allowed to pass it.

Okay I see. I'll leave it as-is now since it works and I rather not do
accidental replacements. :)

Collin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Refactoring rewrite_filename functions
  2024-04-17  2:08             ` Collin Funk
@ 2024-04-17 14:03               ` Bruno Haible
  0 siblings, 0 replies; 12+ messages in thread
From: Bruno Haible @ 2024-04-17 14:03 UTC (permalink / raw)
  To: bug-gnulib, Collin Funk

Hi Collin,

> This will require some code reordering first though. The
> GLFileAssistant used by GLImport is not defined until
> GLImport.execute(). The file name transformations are done in
> GLImport.prepare().

Good point.

> I think the correct solution there is to define the GLFileAssistant in
> GLImport.__init__() with it's default arguments and then set them
> later.

Hmm, this sounds clumsy.

Can't you order things in such a way that
  1. GLConfig is created.
  2. The configuration is elaborated and stored in GLConfig.
  3. GLConfig is finished.
  4. GLFileAssistant is initialized.
?

Bruno





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-04-17 14:04 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-15  1:23 gnulib-tool.py: Remove a redundant function Collin Funk
2024-04-15  2:20 ` Collin Funk
2024-04-15 14:58   ` Bruno Haible
2024-04-15 15:24     ` Collin Funk
2024-04-16 15:09       ` Bruno Haible
2024-04-16 16:23         ` Collin Funk
2024-04-17  1:12         ` Refactoring rewrite_filename functions Collin Funk
2024-04-17  1:35           ` Bruno Haible
2024-04-17  2:08             ` Collin Funk
2024-04-17 14:03               ` Bruno Haible
2024-04-15 11:47 ` gnulib-tool.py: Remove a redundant function Bruno Haible
2024-04-15 14:07   ` Collin Funk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).