Closed windoverwater closed 1 year ago
Yes, it should be in the vtp.cli
directory. There shouldn't be any argument parsing or argument strings outside of vtp.cli
. This means the constructors would change to take parameters as noted in this review comment and in this discussion comment on #60.
As I said in the second comment I want this change because without it I have to do boilerplate extra work to write tests. I also don't think you need to make it: the best way for me to explain it is to make a branch with it. I had already started one to get the tests going. Let me get it to you and you can review.
The plan is to make minimal changes:
vtp.cli
.Alter operations classes to take parameters, but not individual ones. The simplest to do this is to pass the Namespace
object to the __init__
methods of operations instead of the parsed string. Only slightly more work is to convert the Namespace
into a dictionary. The reason to do this is to take advantage of Python keyword arguments (the **
operator) to incrementally convert the parameters explicitly.
def a_function(**keywords): pass
can gradually be changed into:
def a_function(flag=True, level=2, **keywords): pass
This is a tried and tested idiom.
Well, I have a concern with using **keywords. When I tried that, I very quickly screwed myself because of the lack of type checking. Disclaimer - I don't care for unspecified optional or keyword args, so I am starting out biased against that solution.
Regardless, when keywords became a problem (because they were not checked and I wasted time), it became design pattern frustrating since using keywords do not 'phase contain the bug' - it allows a keyword value or type mismatch to continue until something else blows up. So I started down the path of specifying the keys (like you show above). However, that denormalized the arguments between the argparse variant and the dictionary variant. I had to create matching sets of dict keys and argparse entries. When I tried to document the argparse variant and the dictionary variant, I perceived evil flowing out of my fingertips into the code base :-)
I hope you are laughing - at least that is the intention here :-)
Closing this with the merge of feature/looking-into-argparse-again as that branch addresses this.
See https://github.com/TrustTheVote-Project/VoteTrackerPlus/pull/86
Capturing the top level issue raised during conversations regarded https://github.com/TrustTheVote-Project/VoteTrackerPlus/pull/60 that were not addressed when PR 60 was merged.
And that is the question of where the argparse function should reside, either in the cli directory or the ops directory.
See https://github.com/TrustTheVote-Project/VoteTrackerPlus/pull/60 for the 3 conversations not resolved.