Reduce coupling to argparse, and separate Endpoint definitions from the provided parameters. #2021
Conversation
# Conflicts: # .gitignore
|
|
||
| class ExecutionContext: |
There was a problem hiding this comment.
This is not a requirement, but there isn't a major benefit in storing these properties within a class. It merely adds an additional layer between the values and things accessing them.
| args = parser.parse_args() | ||
|
|
||
| # Get the endpoint that the parsed args specify | ||
| endpoint = get_endpoint(args=args, execution_context=e) |
There was a problem hiding this comment.
In my opinion, the contents of the ExecutionContext class appear to be more like static, library-level artifacts that don't require to be contained within an instantation of a class. Should the user desire dynamic additions to the client library, these can still be added dynamically.
| @@ -0,0 +1,26 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
I've provided some tests to aide in maintaining compatibility through the refactoring that I have done here, but I have not modified the circleci configurations to have them run.
| # To be able to make long-running requests to cruise-control | ||
| from cruisecontrolclient.client.Responder import CruiseControlResponder | ||
|
|
||
|
|
||
| def get_endpoint(args: argparse.Namespace, |
There was a problem hiding this comment.
This method contains much more than 'getting the endpoint', so I've partitioned it into more targeted methods that further allow the decoupling of the argparse.Namespace class being used to carry all the parameters.
| @@ -65,5 +65,5 @@ class SubstatesParameter(AbstractSetOfChoicesParameter): | |||
| 'args': ('--substate', '--substates'), | |||
| # nargs='+' allows for multiple of the set to be specified | |||
| 'kwargs': dict(help=description, metavar='SUBSTATE', choices=lowercase_set_of_choices, nargs='+', | |||
| type=str.lower) | |||
| type=str.lower, default='executor') | |||
There was a problem hiding this comment.
Currently, this is set in the __init__ method of the StateEndpoint definition, but I think it's more appropriate to handle the definition a layer higher which surface it to the user of the cli.
|
Hi @twcurrie , thanks for the change and nice explanations/descriptions in this PR! I took one pass on the PR and it looks like it is a purely refactor without any function change. The refactor looks reasonable to me. I'll leave some questions/comments here:
|
This PR is a proposal for the issue discussed in #2020.
I understand the contribution guidelines recommend an issue for some conversations before any changes, and especially discourage large commits, but I'm surfacing this PR to support the discussion for that referenced issue. Many of the elements here could be implemented independently to improve the usability of this Python library. I've done the following:
get_endpointfunction into more focused, reusable pieces. This function contains many operations and is the main source of tight coupling between theargparselibrary and instantiation of Endpoint classes. Through breaking it into pieces, it exposed me to many of the other design decisions within this library that I've touched.ExecutionContext, but leave it's module. Shifts the class attributes contained within it to be merely directly importable static parameters. The notion of the "ExecutionContext" as a class definition doesn't seem necessary, as the class almost exclusively carries class-level properties and few to no properties are stored within instances of it to be referenced later. In it's current execution model, as a command-line tool, the ExecutionContext itself exists only once.__init__methods ofAbstractEndpointimplementations. This complicates the process users have to go through to define new endpoints and it triggers the need to have additional logic handling (specifically this block, tested in this commit)Endpointdefaults defined within the__init__method into the argparse level default.ParameterSetthat's an implementation ofMutableSet, which stores parameters for endpoint requests. It tracks the instanitiated parameters and adhoc parameters. The original author of the library did a lot of "future-proofing" of the library, so this attempts to continue to support it through this distinction.Overall, the choice of dynamically generating the command-line parsers and arguments was an interesting one. It certainly looks like it makes it easier to maintain and extend things for the intended use case of a cli for a REST API, but it's underlying classes could have been made more extendable and testable.