Re: WIP patch to reduce rewrite_filename duplication

[Collin Funk]

Hi Bruno,

Thank you for the detailed explanations as always.

On 4/25/24 1:48 AM, Bruno Haible wrote:
> Object-oriented programming is not easy, and it comes with constant
> hesitations and deliberations.

:)

My least favorite part of my degree was object-oriented design and
database normalization, for similar reasons.

> Another, bigger warning sign is that you can no longer easily explain
> what the GLFileTable is.
> Before, it was a data structure that holds file lists during an operation.
> After, it is a helper class that holds file lists and also meddles with
> file name rewriting as required by the caller. (That's the best description
> I can come up with!)

I see what you mean. I guess the strange setters would be a symptom of
that warning sign that GLFileTable is doing to much.

The way you described the GLFileTable reminds me of the reasoning for
adding the 'dataclasses' module added in Python 3.7 [1]. Since we are
both very big fans of decorators of course. :)

> IMO, the creation of a rewrite_file_name method (singular! you mentioned
> a couple of days ago that the ability to rewrite a single file name is
> one of the motivations for this refactoring) in a central place goes in
> the correct direction. However, moving knowledge about 'cache' and 'config'
> into GLFileTable does not.

Sure, I agree. Though I'm not sure the best place to put it. All of
our non-private functions are in constants.py, but this function seems
out of place there. Since all of the functions there are string
helpers and such. In other words, no GL* classes are imported there.

Maybe it would be best to create a functions.py file?

> Neither of the two proprosals smells well. Why not add a boolean
> 'also_tests_lib' parameter to the rewrite_file_name method, that tells
> whether to replace 'tests=lib/' or not? This way, the refactoring does
> not introduce a functional difference.

Sorry, I think I worded that poorly. Since 'tests=lib/' is internally
used I was thinking that maybe we should warn/error if it is used. I
guess that isn't a problem worth losing sleep over.

For the actual function it would be something like this:

    def rewrite_file_name(config: GLConfig,
                          file_name: str,
                          also_tests_lib: bool = False) -> str:
        ...
        if also_tests_lib and file_name.startswith('tests=lib/'):
           # OK

Since 'tests=lib/' is used internally I felt it makes more sense to
default it to False. That way the call in main.py can ignore it and
the calls in GLImport.py and GLTestDir.py can use it internally.

> In a case like this, it's better if you post the patch for review, rather
> than push it too quickly. Trust me: OO takes a long time to learn.

Yes, I agree.

[1] https://docs.python.org/3/library/dataclasses.html#module-dataclasses

Collin