Hi Bruno,
On 3/28/24 4:05 PM, Bruno Haible wrote:
> Since the program uses a mix between OOP and functional style, it is
> reasonable to think about whether a certain data path is better implemented
> through OOP (with an object holding a private sub-object) or better done
> in a functional style (by passing function parameters).
>
> When doing so, two considerations should be evaluated:
> 1) Is the sub-object really private?
> 2) Do the sub-objects need to be shared? I.e. is it OK to have multiple
> instances? Or is it better to have one instance only?
Thanks for all of the feedback and explanations.
I agree with your points. Most of this was caused by a
misunderstanding that I had. For some reason I was under the
impression that the makefiletable would have to be returned.
I was not a fan of adding another value to the left side of this
assignment:
emit, uses_subdirs = self.emitter.lib_Makefile_am(basename, ...)
This is not necessary since the '.pop()' function modifies the list
contained in the GLMakefileTable. In any case, we removed uses_subdirs
so I would have much less of a problem with it.
As far as the class relationships go, I sort of overlooked those
lines. They seem to be good indications that a mode, such as in
GLImport, might want to change Makefile variables independent of
GLEmiter. Therefore, keeping it as it is would be the correct
decision.
> That is, I would prefer to revert that part, and keep the GLMakefileTable
> as an explicit argument.
I've attached an updated patch.
CollinFrom 531a82a9f9430821c83a6b310f7e5ad9644cc102 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Thu, 28 Mar 2024 18:20:24 -0700
Subject: [PATCH] gnulib-tool.py: Fix removal of variables from
GLMakefileTable.
* pygnulib/GLMakefileTable.py (GLMakefileTable.pop): New function.
* pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am)
(GLEmiter.tests_Makefile_am): Remove variables from the makefiletable
once they are emitted.
---
ChangeLog | 8 ++++++++
pygnulib/GLEmiter.py | 12 ++++++++----
pygnulib/GLMakefileTable.py | 6 ++++++
3 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 542320be4a..70a5c135c8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-03-28 Collin Funk <collin.fu...@gmail.com>
+
+ gnulib-tool.py: Fix removal of variables from GLMakefileTable.
+ * pygnulib/GLMakefileTable.py (GLMakefileTable.pop): New function.
+ * pygnulib/GLEmiter.py (GLEmiter.lib_Makefile_am)
+ (GLEmiter.tests_Makefile_am): Remove variables from the makefiletable
+ once they are emitted.
+
2024-03-28 Bruno Haible <br...@clisp.org>
gnulib-tool: Drop workarounds for Automake < 1.14.
diff --git a/pygnulib/GLEmiter.py b/pygnulib/GLEmiter.py
index 0bfbfe10e3..e63cf34eb8 100644
--- a/pygnulib/GLEmiter.py
+++ b/pygnulib/GLEmiter.py
@@ -893,7 +893,8 @@ AC_DEFUN([%V1%_LIBSOURCES], [
emit += '# No GNU Make output.\n'
# Execute edits that apply to the Makefile.am being generated.
- for current_edit in range(0, makefiletable.count()):
+ current_edit = 0
+ while current_edit < makefiletable.count():
dictionary = makefiletable[current_edit]
if dictionary['var']:
if destfile == joinpath(dictionary['dir'], 'Makefile.am'):
@@ -903,7 +904,8 @@ AC_DEFUN([%V1%_LIBSOURCES], [
# Since we don't have '.' among SUBDIRS so far, add it now.
val = f'. {val}'
emit += '%s += %s\n' % (dictionary['var'], val)
- dictionary.pop('var')
+ makefiletable.pop(current_edit)
+ current_edit += 1
# Define two parts of cppflags variable.
cppflags_part1 = ''
@@ -1196,7 +1198,8 @@ AC_DEFUN([%V1%_LIBSOURCES], [
emit += 'MAINTAINERCLEANFILES =\n'
# Execute edits that apply to the Makefile.am being generated.
- for current_edit in range(0, makefiletable.count()):
+ current_edit = 0
+ while current_edit < makefiletable.count():
dictionary = makefiletable[current_edit]
if dictionary['var']:
if destfile == joinpath(dictionary['dir'], 'Makefile.am'):
@@ -1206,7 +1209,8 @@ AC_DEFUN([%V1%_LIBSOURCES], [
# But we have '.' among SUBDIRS already, so do nothing.
pass
emit += '%s += %s\n' % (dictionary['var'], val)
- dictionary.pop('var')
+ makefiletable.pop(current_edit)
+ current_edit += 1
emit += '\n'
diff --git a/pygnulib/GLMakefileTable.py b/pygnulib/GLMakefileTable.py
index 17a35ee935..48c3267962 100644
--- a/pygnulib/GLMakefileTable.py
+++ b/pygnulib/GLMakefileTable.py
@@ -62,6 +62,12 @@ class GLMakefileTable(object):
result = self.table[y]
return dict(result)
+ def pop(self, index: int) -> dict[str, bool]:
+ '''Remove the given index from the Makefile table. The removed element is returned.'''
+ if type(index) is not int:
+ raise TypeError(f'indices must be integers, not {type(index).__name__}')
+ return self.table.pop(index)
+
def editor(self, dir: str, var: str, val: str, dotfirst: bool = False) -> None:
'''This method is used to remember that ${dir}Makefile.am needs to be edited
to that ${var} mentions ${val}.
--
2.44.0
