lebrice / SimpleParsing

Simple, Elegant, Typed Argument Parsing with argparse
MIT License
386 stars 47 forks source link

Adding the `simple_parsing.replace` function #212

Closed zhiruiluo closed 1 year ago

zhiruiluo commented 1 year ago

This PR is only to add the replace function that work on nested dataclasses and initialized nested subgroups as discussed in the contribution 2 in issue #197. It is dependent on PR #211.

lebrice commented 1 year ago

Hey there @zhiruiluo , this also seems to contains the changes from #211.

Based on our previous conversation, the plan was for this to only have the replace function, without anything related to subgroups. Then a PR to add the subgroup deserialization fix, and another to add the replace_subgroups function (or to add another argument to replace with the flattened subgroup changes).

Thanks again!

zhiruiluo commented 1 year ago

Hi @lebrice,

  1. I'm using the version of to_dict and from_dict from #211 to implement this replace. I simply merge #211 to this PR. If you mean that I should use the old version of to_dict and from_dict in this implementation, I'll change it.
  2. This PR doesn't switch subgroups while replacing it. It replaces subgroups like a nested dataclass with a definite type. It doesn't expose the ugly __subgroups__@key to user for switching subgroups. However, #211 provides add_selected_subgroup in to_dict and parsing_selection in from_dict which results in the possiblity of injecting subgroups selection info by user through dedicated input of new_values in replace function. If you mean we should restrict that behaviour in this PR, I would say we could swich to the old version of to_dict and from_dict.
zhiruiluo commented 1 year ago

Hey there @zhiruiluo , this also seems to contains the changes from https://github.com/lebrice/SimpleParsing/pull/211. Based on our previous conversation, the plan was for this to only have the replace function, without anything related to subgroups.

I think using the old to_dict and from_dict but with fixing for nested dataclass and removing the test_replace_nested_subgroups is the one you want.

lebrice commented 1 year ago

Hi @zhiruiluo , could you please remove any changes to to_dict and from_dict (anything from #211 ) from this PR? This will be easier to review, since both contributions are independant. Thanks again!

zhiruiluo commented 1 year ago

Hi @zhiruiluo , could you please remove any changes to to_dict and from_dict (anything from #211 ) from this PR? This will be easier to review, since both contributions are independant. Thanks again!

Done.