Remove all mention of unreleased service#17
Conversation
|
|
||
| def create_sigv4_client( | ||
| service: str = 'eks-mcp', | ||
| service: str = 'your-service', |
There was a problem hiding this comment.
I don't think we should do a default service string here
There was a problem hiding this comment.
There needs to be some string as the default service. I suggest the empty string.
There was a problem hiding this comment.
We have https://github.com/aws/aws-mcp-proxy/blob/main/aws_mcp_proxy/utils.py#L54-L83, which ensures we find the correct service string, why can't we leave it without a default? 🤔
There was a problem hiding this comment.
The service string will indeed be found and replaced after the fact if none is given, however, given that it is a positional argument, if it's not given a default value, one must be provided. Otherwise uses like this won't type check.
There was a problem hiding this comment.
Rather set it to None then empty string
21a8039 to
328a837
Compare
|
|
||
| def create_sigv4_client( | ||
| service: str = 'eks-mcp', | ||
| service: str = '', |
There was a problem hiding this comment.
Can we make this required, without default parameter? using "" as the signature code will not work. This will help preventing contributors making mistake
There was a problem hiding this comment.
I think the best solution is to make it optional, since it gets inferred if not explicitly provided by the user anyways. In the latest revision, I've made it more elegant by making the string optional as such.
service: Optional[str] = None,
There was a problem hiding this comment.
328a837 to
cb99eb9
Compare
refactor(sigv4_helper.py): change service string to Optional string and default to None
5b3b2b0 to
d1fafd9
Compare
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.