[#241] fixes defaults from the config path were not being passed

[aliounis]

[#241] fixes the issue where defaults from the config path were not being passed when using subparsers (field_wrapper.py).

Additionally fixes an issue where subdataclasses were requiring every value to be defaulted in the config path, instead of falling back to the default in the dataclass definition if it wasn't (dataclass_wrapper.py). This was an additional bug discovered.

Closes #241

[lebrice]

Hey there @aliounis , thanks for making this PR!

I think this kind of change could benefit from a unit test or two. Do you feel up for the "challenge"? This would essentially just be to add your test-case from the issue in the test/test_subparsers.py file.

I'll reply with an example of what that could look like soon.

[lebrice]

Here's what this kind of unit tests could look like. Feel free to add more tests and/or test cases, in particular for the second bug you mentioned.

import pytest
import json
from pathlib import Path
from dataclasses import dataclass
from simple_parsing import ArgumentParser, parse
from simple_parsing.utils import Dataclass

@dataclass
class DoA:
    a_config: str = "taco"
    """
    what does a want?
    """

    def execute(self):
        return self

@dataclass
class DoB:
    b_config: str = "pie"
    """
    what does b want?
    """

    def execute(self):
        return self

@dataclass
class Subprogram:

    command: DoA | DoB
    """
    Do A or B?
    """

    def execute(self):
        return self.command.execute()

@pytest.mark.parametrize(
    ("config_contents", "command_line_args", "expected"),
    [
        ({}, "doa", Subprogram(command=DoA())),
        ({}, "dob", Subprogram(command=DoB())),
        ({}, "doa --config_path <config_path>", Subprogram(command=DoA())),
        ({}, "dob --config_path <config_path>", Subprogram(command=DoB())),
        ({}, "doa --config_path <config_path>", Subprogram(command=DoA())),
        (
            {
                "command": {
                    "a_config": "taco cheese",
                }
            },
            "doa --config_path <config_path>",
            Subprogram(command=DoA(a_config="taco cheese")),
        ),
        (
            {
                "command": {
                    "b_config": "apple pie",
                }
            },
            "dob --config_path <config_path>",
            Subprogram(command=DoB(b_config="apple pie")),
        ),
    ],
)
def test_add_config_path_with_subparsers(
    config_contents: dict,
    command_line_args: str,
    expected: Dataclass,
    tmp_path: Path,
):
    """Test for issue 241: https://github.com/lebrice/SimpleParsing/issues/241

    TODO: Given some previous content in a config file, and given command-line args, check that the result is what is expected.
    """
    config_path = tmp_path / "config.json"
    config_path.write_text(json.dumps(config_contents))

    if "<config_path>" in command_line_args:
        command_line_args = command_line_args.replace("<config_path>", str(config_path))

    actual = parse(type(expected), add_config_path_arg=True, args=command_line_args)
    assert actual == expected

[aliounis]

I can try to add a few unit tests but it probably won't be till later this week. Did the existing tests all pass before (it looks like at least one failed in the automated testing and I'm not sure if the changes I made did that or not)?

[lebrice]

Yes, all tests are required to pass for anything to be merged to Master. Therefore, it seems like your changes did break a test or two. I'm here to help, if you need anything, let me know.