make compiler a python function and other small improvements

[dwRchyngqxs]

This is a prerequisite for the AC PR.

[Fiwo735]

Looks good, I went even further and removed more code duplication/bad patterns, please take a look at the commit above.

However, doing so allowed me to also integrate the Callable student_compiler(...) and fake_compiler(...) into the new helper function run_component(...). I show this change in the commit below. Please let me know if it makes sense - I think it makes things simpler, but I'm not sure if there's some purpose for introducing Callable that I don't know of.

[Fiwo735]

I think the latter version has the least code duplication and it still uses the symlink idea you introduced, which is great. In case we do need the Callable compiler, then I'd extend that idea to all components.

[dwRchyngqxs]

Please check my other PR if what you want is to clean the code. It is not the point of this PR, I just integrated some small changes to be match the new TA_test.py in terms of typing and remove the call to glob because they are small harmless improvements, but they are not the main point of this PR (unless concealing the real purpose of the commit is also considered a purpose of this PR which I would argue for).
The point of this PR is making the compiler a Callable (something with a __call__ attribute, aka. a function; but python puts it in collections.abc for some reason and deprecated the one in typing...) so that I can provide a different function in TA_test.py for the AC PR.
Please consider the point of this PR to make your changes. Or make them on my other draft PR (though I wouldn't recommend). Otherwise I'm reverting all your changes. If you want to keep them, please make a new branch and rebase -i on main only picking your commits.

[Fiwo735]

I thought the other PR (#61) is not ready for a review yet. I see the point of the Callable now, are you happy with other suggestions I did (while you re instantiate the removed student_compiler(...))? If so, I think you have overwritten some minor things:

• build(...) docstring, added: "Return True if successful, False otherwise"
• Remove build_dir from some functions' parameters, as it is no longer needed (if you are passing a Callable compiler)
• Rename relevant_files(...) -> get_relevant_files(...)
• run_subprocess(...) docstring: "Returns tuple..." -> "Returns a tuple..."
• run_tests(...) docstring, added: "Returns a tuple of (passing: int, total: int) tests"
• process_result(...), changed:

if verbose:
        print(result.to_log())
        return
if progress_bar:
        progress_bar.update_with_value(result.passed())
return

into

if verbose:
        print(result.to_log())
elif progress_bar:
        progress_bar.update_with_value(result.passed())

Besides, shouldn't the type of both student_compiler(...) and fake_compiler(...) match the typing Callable[[Path, Path, int], subprocess_status]? Currently they vary in number of parameters, as student_compiler(...) has 4 parameters and fake_compiler(...) has 3 parameters (which it uses only 1 of). Wouldn't making both have either 4 parameters or fake_compiler(...) having only 1 parameter be cleaner?

Regardless, I think we should pass the Callable in a nicer way. Instead of:

compiler=fake_compiler if args.validate_tests \
                else lambda test, out, to: student_compiler(build_dir / "c_compiler", test, out, to),

we could

from functools import partial
...
compiler=(
    fake_compiler
    if args.validate_tests
    else partial(student_compiler, build_dir / "c_compiler")
)

Lastly, I think we could have a better name than fake_compiler(...) as that could be confusing to students. Maybe reference_compiler(...)?

[dwRchyngqxs]

are you happy with other suggestions I did

Most of them cover stuff I will change anyway in the other PR and are good here otherwise so they are fine by me. I re applied some of the changes (see coming commit).

Besides, shouldn't the type of both student_compiler(...) and fake_compiler(...) match the typing Callable[[Path, Path, int], subprocess_status]? Currently they vary in number of parameters, as student_compiler(...) has 4 parameters and fake_compiler(...) has 3 parameters (which it uses only 1 of). Wouldn't making both have either 4 parameters or fake_compiler(...) having only 1 parameter be cleaner?

No, the function passed should match the type but the compiler themselves conceptually require these arguments, and any compiler we might want to pass as a function will only need those.

Regardless, I think we should pass the Callable in a nicer way. Instead of:

compiler=fake_compiler if args.validate_tests \
    else lambda test, out, to: student_compiler(build_dir / "c_compiler", test, out, to),

we could

from functools import partial
...
compiler=(
    fake_compiler
    if args.validate_tests
    else partial(student_compiler, build_dir / "c_compiler")
)

As a functional programming aficionado I wanted to do that. But I decided against because was worried of the readability and clarity for standard EEE people.

[dwRchyngqxs]

Lastly, I think we could have a better name than fake_compiler(...) as that could be confusing to students. Maybe reference_compiler(...)?

fake_compiler is somewhat descriptive, reference_compiler gets the point across but is implying there is a compiler. I think something like copy_reference_compiler is even more descriptive and doesn't imply a real compiler is actually called.

[Fiwo735]

As a non-functional programmer, I still see partial(...) as easier to understand (than lambda, which students would also be unfamiliar with), so feel free to use that 😆

As for fake_compiler(...) name, I think symlink_reference_compiler(...) would convey your intention best then, as it literally symlinks (and not copies).

[Fiwo735]

Looks good, feel free to merge