add options to bucket and keys#93
add options to bucket and keys#93certainmagic merged 3 commits intoBackblaze:masterfrom fmbz:Backblaze/fmbz/add-options-to-bucket-and-app-keys
Conversation
| @B2Json.optional | ||
| private final Long expirationTimestamp; | ||
|
|
||
| @B2Json.required |
There was a problem hiding this comment.
generally, we don't add required to structures that we've already shipped. i'm trying to think through why we think this is safe to do...
There was a problem hiding this comment.
Agree with ab, better to make this optional and not have to think about whether this is safe :)
There was a problem hiding this comment.
was an oversight - updated
| (bucketInfo == null ? 0 : bucketInfo.size()) + " infos," + | ||
| (corsRules == null ? 0 : corsRules.size()) + " corsRules," + | ||
| (lifecycleRules == null ? 0 : lifecycleRules.size()) + " lifecycleRules," + | ||
| (options == null ? 0 : options.size()) + " options," + |
There was a problem hiding this comment.
Wondering about listing the options here. Would that be reasonable
| "expirationTimestamp, " + | ||
| "options" | ||
| ) | ||
| public B2ApplicationKey(String accountId, |
There was a problem hiding this comment.
Incompatible change? Does this need a major version number bump?
There was a problem hiding this comment.
the B2Json.constructor annotation can only be used once per class.
Looks like there isn't a unit test for this class. Fabian, can you please create one for it? Running that test should result in an error due to having two constructor annotations
There was a problem hiding this comment.
i'm fine with bumping the version. in fact, fabian & i just looked and we've bumped to v4 because of other little (breaking) changes and haven't released yet, so bumping is free. :)
There was a problem hiding this comment.
re: unit test malay suggested -- one particularly good test is to make a structure, toJson it, fromJson it, and verify that the original and the copy match with equals. you can also toJson again to be sure that the two json versions also match, but i'm not sure that's strictly necessary most of the time (at least not if you trust your equals method).
There was a problem hiding this comment.
thanks for suggestion - updated
| "expirationTimestamp, " + | ||
| "options" | ||
| ) | ||
| public B2ApplicationKey(String accountId, |
There was a problem hiding this comment.
the B2Json.constructor annotation can only be used once per class.
Looks like there isn't a unit test for this class. Fabian, can you please create one for it? Running that test should result in an error due to having two constructor annotations
| "expirationTimestamp, " + | ||
| "options" | ||
| ) | ||
| public B2ApplicationKey(String accountId, |
There was a problem hiding this comment.
i'm fine with bumping the version. in fact, fabian & i just looked and we've bumped to v4 because of other little (breaking) changes and haven't released yet, so bumping is free. :)
| ); | ||
| } | ||
|
|
||
| @B2Json.constructor(params = "accountId,bucketId,bucketName,bucketType," + |
There was a problem hiding this comment.
as Malay noted above, we can only have on @B2Json.constructor annotation.
| "expirationTimestamp, " + | ||
| "options" | ||
| ) | ||
| public B2ApplicationKey(String accountId, |
There was a problem hiding this comment.
re: unit test malay suggested -- one particularly good test is to make a structure, toJson it, fromJson it, and verify that the original and the copy match with equals. you can also toJson again to be sure that the two json versions also match, but i'm not sure that's strictly necessary most of the time (at least not if you trust your equals method).
certainmagic
left a comment
certainmagic
left a comment
There was a problem hiding this comment.
thanks! especially for making the changelog. :)
…rivate Nbehrens/merge public back to private
4 participants
No description provided.