added import comments #654
Conversation
googlebot
added
the
cla: yes
daniel-sanche
force-pushed
the
kms_import_comments
branch
from
cfe8b11 to
a3b34ec
Compare
|
|
||
| // getAsymmetricPublicKey retrieves the public key from a saved asymmetric key pair on KMS. | ||
| // | ||
| // Requires: |
There was a problem hiding this comment.
These lines are a little bit weird since they don't list every import, only some of them. Would you mind splitting all of these functions into separate files and include the import() block in the region tag? I think that's the pattern the canonical snippet will follow, so it would be a step in the right direction.
There was a problem hiding this comment.
I've had conversations with Kurtis (GoogleCloudPlatform/java-docs-samples#1218) and the product team (b/113025156) on how best to handle this. We decided that until we have an agreed upon standard way of handling imports, it would be best to leave the imports in the comments for now. This gives the users the information they need, and it's easy to be consistent with how we present the information across all supported languages without significant refactors.
As for not including all imports, I assumed standard imports like fmt, errors, and context would be considered a given for anyone familiar with go, but I can add them if you think it would be useful
There was a problem hiding this comment.
I'm not sure. These are all standard library imports. So, goimports should find them. So, I'm not sure this is worth adding.
If we decide on the right way to include imports in the canonical snippet, we can update this.
Does that make sense?
There was a problem hiding this comment.
I suppose. I'd prefer to include these for consistency sake since these comments are needed for python and java. Even in go it might not be clear whether we're using standard crypto libraries or 3rd party ones with similar names, so I'd prefer to be explicit. But I guess these aren't as necessary for go
I am going to want to remove the region tags from the import block though, so this PR should stay open either way
3 participants
added imports to snippet comments (b/113025156)