Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic repository #377

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MatthewHambley
Copy link
Collaborator

@MatthewHambley MatthewHambley commented Jan 18, 2025

Rather than requiring double-entry book-keeping use introspection to automatically build up the list of tools.

This change involves moving everything which is not a tool out of the tool directory.

Also, since the fundamental job of fab is to shell out to executables it seems like a good idea to use a dedicated pytest extension for mocking the subprocess system rather than doing it ad-hoc each time.

@MatthewHambley MatthewHambley marked this pull request as ready for review January 18, 2025 12:30
@hiker
Copy link
Collaborator

hiker commented Jan 18, 2025

I just saw this PR, and I have several huge issues with it:

  1. It combines at least two totally unrelated changes: adding the mock tool (which sounds like a great idea) and some introspection feature.
  2. There doesn't appear to be a ticket to discuss this introspection feature, and because of the size of this patch it makes it hard to read.
  3. This PR does not follow our development process: PRs should go into develop, and then being merged into main. This patch ignores 6 or 7 PRs that are already in develop, and will very likely cause conflicts in a lot of them, since we are actively working on the files that are changed/moved in this PR.
  4. I actually don't agree with some of the changes, e.g. moving Tool to __init__.py. I certainly prefer to have each class in a file of the same name (unless it's helper classes or so, and I admit I need to fix up a few files that have grown too much over time .. compiler.py, I look at you :) ... though, having stand-alone files for all compiles is also a it annoying, they are tiny). While I could quote https://docs.python-guide.org/writing/structure/#packages ("A commonly seen issue is adding too much code to init.py files."), but I admit I am not that strictly following these rules. But in my opinion I __init__.py should manage packages, not contain code.
  5. I don't consider the list of classes in __init__.py double-entry bookkeeping, or at least I don't consider it bad: the idea is that this selects the classes from that subpackage that should be used externally. For example, CompilerSuiteTool doesn't make too much sense to be used except in the C- and Fortran compiler base class, and I intend to remove it from init at some stage)
  6. I also prefer not to use the filenames (e.g. from fab.tools.ar import Ar). The __init__.py file can be use to hide implementation details. Admittedly, I doubt we will ever rename ar.py. From my point of view, the __init__.py file defines the API for the user of a sub-package. The one exception I see is to avoid cyclic dependencies, that might sometimes make it necessary to import a file.
  7. I am not sure if I like having ToolBox, ToolRepository, Category in the 'root' source folder of fab, that directory might end up contain quite a few files. From my point of view, they are all tool related classes, and they all together make a very neat, self-contained sub-package. To explain what I mean: atm you could take the whole tools directory, move it (e.g. into a different project), and you should have all you needed. With this change, you now have to copy other files from outside the directory, and files in tools import more things from outside. I just checked, atm we have exactly two outside imports in tools (string_checksum and BuildConfig, the latter only for typing). I would at least like to have a discussion about this.

Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As indicated in my comment, this PR should be split into two (change to testing, then the rest), and be based on the develop branch. I would also like to have a discussion about the direction this refactoring is going, e.g. moving files out of the tools directory kind of breaks this subdirectory, since it now needs outside imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants