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.

Sign up for GitHub

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

Jump to bottom

Rework of the subgroups feature + Minor Refactoring #185

Merged
merged 48 commits into from
Jan 10, 2023
Merged

Rework of the subgroups feature + Minor Refactoring #185

merged 48 commits into from
Jan 10, 2023

Conversation

lebrice
Copy link
Owner

@lebrice lebrice commented Dec 13, 2022 •
edited
Loading

WIP.

  • Add a whole bunch of tests for the subgroups feature (with nesting)
  • Make nested subgroups work
  • Make other tests work
  • Identify and mark out candidates for subsequent refactoring PRs. (out of scope, this is already a pretty large PR as-is).

@lebrice lebrice changed the title Rework of the subgroups feature + ArgumentParser Refactoring Rework of the subgroups feature + Refactoring Dec 13, 2022
@lebrice lebrice mentioned this pull request Dec 19, 2022
Closed
@lebrice lebrice marked this pull request as ready for review January 5, 2023 21:23
@lebrice
Adding test with current WIP implementation
fab0f66 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Removed the previous subgroups implementation
facfc99 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix test collection for vscode
815b2b4 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Making some progress (need to fix postprocessing)
e662ebe 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Making more progress, almost there (ish)
0b2bb3b 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Making more progress (need to fix conflicts)
efea1cd 
The FieldWrapper is created for the subgroups arg.
I now create a new DataclassWrapper and graft add it as a child of the
DataclassWrapper that contains the subgroup field.

The current issue seems to be that the ConflictResolver is seeing both
the FieldWrapper and the DataclassWrapper (I Think) as pointing to the
same destination!

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Conflicts confusion is clearing up
3d51edb 
The `conflict_resolver.resolve` method was actually doing two things:
- Resolving conflicts
- Flattening the list!

This was causing confusion, since `wrappers` was secretely growing,
which was causing the issue with duplicates from before.

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Things are parsing, but default is slightly wrong
690e3bb 
This is getting to the point where things are parsing, but the default
value for a part of the subgroup is wrong. Need to check what exactly is
going on.

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
'subgroups' is only added on namespace when needed
a6253a7 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Progress: Might have identified issue
0d5db49 
I think I figured out the issue. If we generate a DataclassWrapper
for a subgroup, then it doesn't get a real `Field`, which might be
preventign the parsing from working properly?

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Refactor _set_instances_on_namespace
f28ff13 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Refactor, fix some failing basic tests
1870903 
Certain things were breaking in basic tests.

TODOs left:
- Still getting some errors in the ALWAYS_MERGE case
- Still not parsing the right values for the other arguments

Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix small bug in _consume_constructor_arguments
2b1872f 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Refactoring: Improve naming of Parser methods
fa7a1d4 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
[dirty] wip (switching machines)
f722394 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix mistake with self._wrappers, few tests left
d583366 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Simplify subgroups fn, begin rewriting tests
3650f51 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Simple subgroup tests are passing
9bd0069 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix issue with .setdefault and new subgroups
386720c 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix required arg for equivalent_argparse_code
bd46ca9 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Remove a logger.Critical call
2c011c1 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Remove hacky code from previous iteration
40283cf 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Remove the _print_tree function
ae45549 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Make first block of logging in test a bit better
9e1539c 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Apply pre-commit hooks to field_wrapper.py
9a701a6 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Cleanup the resolve_subgroups code a tiny bit
0f9c2ee 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Make Dataclass an actual Protocol
235b714 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Make DataclassWrapper use Dataclass typevar
73e8ea6 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Add test to check that subgroups are saved (#139)
4595cd6 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Make subgroups sig reflect current limitations
ab81b26 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Add more tests (not passing)
81ba977 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Refactor FieldWrapper.default
0ccbcd4 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix required subgroups issue
61e0ff2 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix error in test for subgroups with a conflict
bd2f895 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Minor esthetic change to test_subgroups.py
aef61f2 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix issues with py37 and parsing of Reused lists
f1fdff2 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Very minor improvement to unrelated test
f94a274 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice lebrice changed the title Rework of the subgroups feature + Refactoring Rework of the subgroups feature + Minor Refactoring Jan 5, 2023
@lebrice
[optional] Make the "reuse" logic a bit better?
b8787c8 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Revert "[optional] Make the "reuse" logic a bit better?"
5cd4270 
This reverts commit b8787c8.
@lebrice
Fix typing of the subgroups function
30b92ae 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Removed unused _remove_help_action and test
448d4b1 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
"create_dataclasses"->"instantiate_dataclasses"
cf6ac13 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Remove commented subgroup tests
dcc8808 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Add more tests for the subgroups function
ea1e524 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Copy link
Owner Author

lebrice commented Jan 6, 2023

Potential future improvements:

  • Show all possible subgroups in the "--help" action, not just the selected subgroups.
  • Make the subgroups function more flexible:
    • Allow passing partials or lambdas as default factories or subgroup options

@lebrice
Use dict instead of Mapping
1dac691 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Add test for help string future feature
e2a73bf 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Fix small bug in test
8a4e183 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice
Merge branch 'master' into nested-subgroups
73c9cd1 
Signed-off-by: Fabrice Normandin <fabrice.normandin@gmail.com>
@lebrice lebrice merged commit 6d3d40a into master Jan 10, 2023
@lebrice lebrice deleted the nested-subgroups branch January 10, 2023 23:11
This was referenced Jan 11, 2023
Closed
Closed
@lebrice lebrice mentioned this pull request Jan 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@lebrice